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.

Sign up for GitHub

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

Jump to bottom

default value for option/argument (and show it in the help text) #389

Merged
merged 10 commits into from Nov 7, 2020
Merged

default value for option/argument (and show it in the help text) #389

merged 10 commits into from Nov 7, 2020

Conversation

scott-xu
Copy link
Contributor

Fix #82

@scott-xu
default value for option (and show it in the help)
7efd9c2
@scott-xu
Fix UT
27690f9 
Note: `UseConstructorInjection` must call prior to `UseDefaultConventions` since we now need to access model at early stage
@scott-xu
Support DefaultValue for CommandArgument
e4eb07e
@scott-xu
Do not show default value for (bool,T)
5baac23
@scott-xu scott-xu marked this pull request as ready for review August 30, 2020 05:30
@scott-xu scott-xu changed the title default value for option (and show it in the help) default value for option/argument (and show it in the help text) Aug 30, 2020
@natemcmaster
Copy link
Owner

Looks like a good start. If you can resolve merge conflicts and also update the behavior of Value/HasValue per #82 (comment), I'd be happy to accept this one!

@scott-xu scott-xu marked this pull request as draft September 14, 2020 01:38
@scott-xu scott-xu marked this pull request as ready for review September 14, 2020 15:09
@scott-xu
Copy link
Contributor Author

Merge conflicts are resolved and the behavior of Value/HasValue has been updated.

natemcmaster
natemcmaster requested changes Sep 22, 2020
View reviewed changes
Copy link
Owner

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. Looks like we're getting closer, but there are a few behavioral changes I think we should address first. Thanks for your persistence on this one!

src/CommandLineUtils/CommandArgument.cs Outdated Show resolved Hide resolved
src/CommandLineUtils/CommandOption.cs Outdated Show resolved Hide resolved
test/CommandLineUtils.Tests/ConstructorInjectionConventionTests.cs Outdated Show resolved Hide resolved
src/CommandLineUtils/Conventions/OptionAttributeConventionBase.cs Outdated Show resolved Hide resolved
@scott-xu scott-xu marked this pull request as draft September 22, 2020 12:26
@scott-xu scott-xu marked this pull request as ready for review September 26, 2020 09:02
@scott-xu
Copy link
Contributor Author

@natemcmaster I'd say, all your 3 suggestions make great sense to me. Thanks for the careful review. I've changed accordingly.

natemcmaster
natemcmaster approved these changes Nov 7, 2020
View reviewed changes
Copy link
Owner

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Sorry for the slow response while I took a break from open source. I'm working on getting a 3.1 version released soonish, and this will be part of it

@natemcmaster natemcmaster changed the base branch from main to feature/default-value November 7, 2020 18:32
@natemcmaster natemcmaster merged commit ea5922d into natemcmaster:feature/default-value Nov 7, 2020
@natemcmaster natemcmaster added this to the 3.1.0 milestone Nov 7, 2020
@natemcmaster
Copy link
Owner

I'm merging this to a feature branch. There are a few additional changes I want to make, and will add them on top of your work. Thank you again @scott-xu !

@natemcmaster natemcmaster modified the milestones: 3.1.0, 4.0.0 Nov 7, 2020
@natemcmaster natemcmaster mentioned this pull request Jan 11, 2021
Merged
@scott-xu scott-xu deleted the default-value-for-help-text branch January 18, 2021 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natemcmaster natemcmaster natemcmaster approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.

Feature request: provide default value for option (and show it in the help)
2 participants
@scott-xu @natemcmaster