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

Fix ArgMatcher consuming extra arguments when max_values is set #1481

Closed
wants to merge 1 commit into from

Conversation

sphynx
Copy link
Contributor

@sphynx sphynx commented May 28, 2019

Closes #1480

@sphynx
Copy link
Contributor Author

sphynx commented May 28, 2019

I had to change error type in one of doc-string tests from TooManyArguments to UnknownArgument. I believe that it's more consistent this way, because now if we pass 2 arguments when max_values = 1(i.e. one more than needed) - we get TooManyArguments, but if we pass 3 arguments to the same option -- we already get UnknownArgument for the third one (while it's conceptually still TooManyArguments). I demonstrated this situation in two extra tests, included in this PR.

I believe that it's a good idea to guide the process of matching arguments by the information from max_values and min_values.

@kbknapp
Copy link
Member

kbknapp commented Oct 30, 2019

@sphynx Sorry for the delay and thank you for the PR. If you're still interested in this PR could you please rebase on the latest master (which now contains the v3-beta code)? It looks like the third test works natively on v3, but test 1 and 2 still fail (2 with TooManyValues instead of the correct UnknownArgument and 1 due to MissingRequired instead of the correct UnkownArgument)

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.

ArgMatcher with max_values consumes one more argument than it should
3 participants