Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated to pyyaml 6.0 and generate-local-access-key script and remove access_key.py/json #1840

Closed

Conversation

dmichaels-harvard
Copy link
Contributor

  • Added generate-local-access-key script (from snovault) to pyproject.toml;
    orignally created for smaht-portal since early in development no way to
    create an access-key normally using the UI; but generall useful/convenient.
  • Removed types/access_key.py and schemas/access_key.json as the ones in snovault are sufficient.
  • Changed pyyaml version to 6.0.


* Added generate-local-access-key script (from snovault) to pyproject.toml;
orignally created for smaht-portal since early in development no way to
create an access-key normally using the UI; but generall useful/convenient.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
create an access-key normally using the UI; but generall useful/convenient.
create an access-key normally using the UI; but generally useful/convenient.

@@ -122,7 +122,7 @@ def anon_pipeline():


def run(pipeline, inpath, outpath):
for item_type in ORDER:
for item_type in ORDER():
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my related comment on your snovault PR, but I found it really weird to see (a) a function here that is all-uppercase, and (b) a change in the semantics of ORDER, which had previously been a list. I think if the semantics changes in so strong a way, it should just have a different name, so that it doesn't look to someone who doesn't know about the name like you've used it wrong.

So my suggestion is to rename the function to something like load_order_for_app in the other PR and then use the newly chosen name here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants