Skip to content

nargs=-1 works with envvar #1604

Merged
davidism merged 1 commit intopallets:masterfrom
MLH-Fellowship:envvar-nargs-bug
Aug 6, 2020
Merged

nargs=-1 works with envvar #1604
davidism merged 1 commit intopallets:masterfrom
MLH-Fellowship:envvar-nargs-bug

Conversation

@rch-liu
Copy link
Contributor

@rch-liu rch-liu commented Jun 25, 2020

This PR aims to fix two bugs:

  1. Initially combining nargs=-1 and envvar would yield an empty tuple where the expected behaviour of nargs=-1 is to take on an unlimited number of arguments passed in envvar. @amy-lei and I added a fix so that now combining nargs=-1 with envvar should output all the expected values from envvar rather than an empty tuple.

  2. We also added error catching for nargs > len(envvar) where we would raise: Got unexpected extra arguments and 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

@rch-liu rch-liu changed the title Fixe #729: Resolves bug related to mixing nargs=-1 and envvar Fix #729: Resolves bug related to mixing nargs=-1 and envvar Jun 25, 2020
@amy-lei
Copy link
Contributor

amy-lei commented Jun 25, 2020

We discovered that nargs was also not enforced when the values came from default, so we addressed/added tests for it as well.

nargs > 1 is validated for envvar and default values

Co-authored-by: Rachel Liu <r248liu@uwaterloo.ca>
@davidism
Copy link
Member

davidism commented Aug 6, 2020

Refactored the tests to use parametrize. Simplified the nargs validation so it uses the same message for any combination, rather than reusing the argument message, which didn't really fit.

@davidism davidism changed the title Fix #729: Resolves bug related to mixing nargs=-1 and envvar nargs=-1 works with envvar Aug 6, 2020
@davidism davidism merged commit 82ca854 into pallets:master Aug 6, 2020
@davidism davidism deleted the envvar-nargs-bug branch August 6, 2020 01:58
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't mix nargs=-1 with envvar

3 participants