-
Notifications
You must be signed in to change notification settings - Fork 413
Fix some help and GetRequiredValue issues
#2601
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 some help and GetRequiredValue issues
#2601
Conversation
adamsitnik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for fixing all the issues @jonsequitur !
| switch (this) | ||
| { | ||
| case Argument<bool> boolArgument: | ||
| boolArgument.DefaultValueFactory = _ => false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we can define a static lambda to reuse it for all other boolean arguments
| boolArgument.DefaultValueFactory = _ => false; | |
| boolArgument.DefaultValueFactory = static _ => false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subjective nit: I am not convinced that switch is the best construct in this case, as it's unlikely that we need other types with such special handling (if should be just fine).
It would be also nice to add a comment explaining why it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, if is cleaner. I was considering whether other types should have similar treatment.
src/System.CommandLine/Argument.cs
Outdated
|
|
||
| internal static Argument None { get; } = new NoArgument(); | ||
|
|
||
| internal class NoArgument : Argument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: since all the callers use Argument.None, could it be made private?
| internal class NoArgument : Argument | |
| private sealed class NoArgument : Argument |
|
|
||
| var result = root.Parse("-v -v -v"); | ||
|
|
||
| using var _ = new AssertionScope(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for my education: why do we need it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the behavior of the assertions so that all will be evaluated and reported even if the first one fails. Multiple assertions in a test is often considered an antipattern because they can fail independently and failing the test on the first assertion failure loses useful information.
The AssertionScope approach provides a way to make sure all assertion results are reported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the behavior of the assertions so that all will be evaluated and reported even if the first one fails. Multiple assertions in a test is often considered an antipattern because they can fail independently and failing the test on the first assertion failure loses useful information.
The
AssertionScopeapproach provides a way to make sure all assertion results are reported.
Thanks a lot! It would be great if we could enable it project-wide without a code change (like a csproj setting)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could be done using a base class (to instantiate this in the constructor and dispose it in Dispose) but I find in well designed tests, multiple assertions aren't the norm and I prefer not to encourage this. One scenario per test should usually guide tests toward one assertion per test.
76d940d to
e96ecb3
Compare
This fixes the following issues:
The approach I took to fix #2573 makes
Argument<bool>andOption<bool>have an implicitDefaultValueFactory. This change drove changes to theHelpBuilderso I also fixed #2582 while I was in there.It's not obvious to me that this is the right fix, so I thought that the PR would help clarify the discussion.
For example, should other types than
boolhave this behavior of an implicit default value?