Merged
Conversation
Contributor
|
We discovered that |
0b08f21 to
921c9fb
Compare
nargs > 1 is validated for envvar and default values Co-authored-by: Rachel Liu <r248liu@uwaterloo.ca>
921c9fb to
3d3ea9c
Compare
Member
|
Refactored the tests to use |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR aims to fix two bugs:
Initially combining
nargs=-1andenvvarwould yield an empty tuple where the expected behaviour ofnargs=-1is to take on an unlimited number of arguments passed inenvvar. @amy-lei and I added a fix so that now combiningnargs=-1withenvvarshould output all the expected values from envvar rather than an empty tuple.We also added error catching for nargs > len(envvar) where we would raise:
Got unexpected extra argumentsand when nargs < len(envvar) we would raise:argument [argname] takes [nargs] values.The changelog is also updated with our new changes. Let us know what you think @davidism!
fixes #729