-
Notifications
You must be signed in to change notification settings - Fork 12
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
Convert env to a list compatible with Galaxy job conf #79
Conversation
Pull Request Test Coverage Report for Build 4137627996Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! @cat-bro and I discussed this today, and we too were thinking that supporting both syntaxes would be the best solution. The simple dict format is compact and easy for a majority of usecases, and the array syntax for more advanced cases.
tpv/core/entities.py
Outdated
def validate(self): | ||
""" | ||
Validates each code block and makes sure the code can be compiled. | ||
This process also results in the compiled code being cached by the loader, | ||
so that future evaluations are faster. | ||
""" | ||
self.convert_env() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to move this to the constructor, at the point of first assignment?
total-perspective-vortex/tpv/core/entities.py
Line 193 in fadf3f6
self.env = env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for the fix. Let me know how urgently you need a release made.
No rush, I can install the dev version. Thank you! |
In Galaxy job destinations/environments,
env
is a list, where each element is a dict with one of three formats:In TPV, env is instead a dict where keys are vars and values are their values. These are converted into a list of
{"name": k, "value": v}
dicts when the job destination is constructed. Because static destinations in the Galaxy job conf are no longer required as of TPV 2.0.0, it's desirable to be able to define destinations and complex envs fully in the TPV config.This change supports the list syntax but is backwards compatible with the dict syntax. It overwrites any values on merging where names/keys match, and preserves the ordering of any existing env entries when merging. Any non-matching env entries are appended.