fix #728 #730
fix #728 #730
Conversation
src/System.CommandLine/Argument.cs
Outdated
| public void SetDefaultValue(object value) => SetDefaultValue(() => value); | ||
| public void SetDefaultValue(object value) | ||
| { | ||
| if (value is null) |
There was a problem hiding this comment.
So I don't get this block here, since this implies calling Argument.SetDefaultValue((object)null) would throw, but Argument.SetDefaultValue(() => null) would be fine, even though I'd expect those to meant the same thing.
There was a problem hiding this comment.
Agreed. I think the API needs to change so that intention is more clear between:
- there is a default value, and it is null, and
- there is no default value.
The latter is not currently representable in the API.
There was a problem hiding this comment.
In the latest commit, I've differentiated SetDefaultValue from SetDefaultValueFactory and added tests to clarify the behaviors. Do you think this helps with the ambiguity?
These APIs still feel fairly provisional though, and a few issues (#581, #704, #723) have pointed toward the need for a more holistic approach to default values, custom conversion, and validation. I won't try to address this here but I suspect it might result in SetDefaultValue and SetDefaultValueFactory becoming obsolete.
Co-Authored-By: Kevin B <Keboo@users.noreply.github.com>
| } | ||
|
|
||
| public Argument(Func<T> defaultValue) : this() | ||
| { | ||
| SetDefaultValue(() => defaultValue()); | ||
| SetDefaultValueFactory(() => defaultValue()); |
There was a problem hiding this comment.
@jonsequitur: it seems the original bug I hit isn't being fixed here: if I pass null in for defaultValue, this will still call SetDefaultValueFactory and ultimately null ref once the property tries being accessed.
There was a problem hiding this comment.
Thanks! I realized I made this change on another branch where I'm refactoring toward fixing #723. These APIs may change significantly by the time that's done but the refactorings are worth merging earlier which will actually fix this issue.
This addresses #728, in part, by preventing
nullfrom being set as the default argument via theArgument.SetDefaultValue(object)overload. Additional design questions here.