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

Add default verb support #556

Closed
wants to merge 16 commits into from
Closed

Conversation

Artentus
Copy link
Contributor

I added basic support for a default verb (a verb that will be assumed if no verb is specified).
Tests all completed on my end but I'm not too knowledgable about this code, if there is any problem let me know.

@moh-hassan
Copy link
Collaborator

Thanks @Artentus for your work.
I did a quick test:

  • for a default verb, it works as expected.
  • With more than default verb (not allowed), an Exception "Sequence contains more than one matching element" is fired.
    You can validate that only one verb has a default and fire error,e.g, "More than one default verb is not allowed"
  • Modify the HelpText screen "--help" to show the help with indication of the default verb, e.g

read (Default verb) Reads from source.
write Writes to destination.

  • Add test cases to cover the above cases.

@Artentus
Copy link
Contributor Author

Artentus commented Jan 4, 2020

The error with more than one default verb is expected. However I do agree a more descriptive error message could be useful.

As for the help text, I'll first have to see how that is actually implemented before I can modify it. Will see what I can do.

@moh-hassan
Copy link
Collaborator

As for the help text, I'll first have to see how that is actually implemented before I can modify it. Will see what I can do.

You can concentrate in the error handling and its unit test and push the modification, so I can start review the PR.

@Artentus
Copy link
Contributor Author

Artentus commented Jan 5, 2020

Done, there is now proper error handling and a couple of new unit tests.

@moh-hassan moh-hassan self-assigned this Jan 5, 2020
@moh-hassan
Copy link
Collaborator

It's nice if you git rebase to origin\develop to sync the last changes in develop branch.

@Artentus
Copy link
Contributor Author

Sorry it took me so long, have been busy this week, but everything is merged now.

I see you don't want to allow default verbs with no name. My reasoning behind this was to just have it behave like no verb at all. Functionally it doesn't change anything tho.

@moh-hassan
Copy link
Collaborator

moh-hassan commented Jan 14, 2020

Thanks @Artentus for Update.
PR is approved and ready to merge

hadzhiyski and others added 5 commits February 2, 2020 20:02
* Add MyGet package provider to consume the daily builds. (commandlineparser#566)

* Options groups take in account default value

* Do not allow options groups and exclusive set names to be used together

* Multiple group errors are shown together

* MissingGroupOptionError compare option names

Co-authored-by: Mohamed Hassan <[email protected]>
@moh-hassan
Copy link
Collaborator

Merged manually to resolve merge confliction at commit 4bed3432c

@moh-hassan moh-hassan closed this Feb 3, 2020
@moh-hassan moh-hassan added this to the 2.8 milestone Feb 27, 2020
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.

4 participants