GetRequiredValue throws for implicit values

[jonsequitur]

Currently, Argument.GetRequiredValue and Option.GetRequiredValue will throw InvalidOperationException when the argument or option wasn't actually parsed from command line input. For example:

var option = new Option<bool>("-x");

var command = new RootCommand
{
    option
};

// the following throws: -x is required but was not provided
var value = command.Parse("").GetRequiredValue(option);

If Option.DefaultValueFactory is set, then the exception is not thrown.

I believe the design of GetValue and GetRequiredValue should focus on whether the value for the symbol can be bound, including implicit and default values, regardless of whether a matching token was found on the parsed commandline.

This could be the wrong intuition though. For bool it seems intuitive, and perhaps for int and other numeric types that default to 0, but for more complex structs such as DateTime, as well as for reference types, this kind of behavior seems nonsensical.

What are people's thoughts on this?

[elgonzo]

I find having both GetValue and GetRequiredValue methods very confusing. Why exactly is the second method having "Required" in its name? Is the required-ness of an Option/Argument value not specified by its arity?

To me personally, it would make more sense for GetValue to return the value from the commandline or default value defined by the argument/option and throw if neither the commandline provides a value nor the option/argument providing a default value. Value-less flag options implicitly have a bool value depending on their presence/absence in the commandline.

GetRequiredValue to me feels more like it should be a TryGetValue method, if i am not mistaken. This should address the concern of avoiding null checks (the apparent reason for the commit introducing the GetRequiredValue method 3bbc68e)

Just my 2 cents...

(Side track: What would/should happen if a user defines a value-less flag Option<bool> that normally has an implicit boolean value, but also specifies a DefaultValueFactory? I know it's silly, but it's also technically and easily realizable. I guess it should behave like any other option with a DefaultValueFactory and an arity of zero, or?)

[jonsequitur]

GetValue<T> has a return value of T? and returns default for the type when no value was provided on the command line or via a DefaultValueFactory. This has always been its behavior and we didn't want to change it. In practice though, it's kind of awkward to use when nullable reference types are enabled and you know that the option or argument has a default value and will never return default for type T. You end up having ! over and over:

var value = parseResult.GetValue(option)!;

GetRequiredValue<T> returns T and throws if it can't provide a value, avoiding this situation.

Is the required-ness of an Option/Argument value not specified by its arity?

Arity is a syntax-level concept that determines how the command line is interpreted. It's decoupled from what .NET type an argument or option returns.

(Side track: What would/should happen if a user defines a value-less flag Option that normally has an implicit boolean value, but also specifies a DefaultValueFactory? I know it's silly, but it's also technically and easily realizable. I guess it should behave like any other option with a DefaultValueFactory and an arity of zero, or?)

Yes, the default can be specified by DefaultValueFactory. So in a way this issue is asking whether there should be a default DefaultValueFactory for Option<bool> that returns false.

[elgonzo]

and we didn't want to change it

You're still deep in beta. :-)

It's a very unusual and surprising choice of APIs, considering that it is a common pattern in .NET to have Get* methods that throw if there is no value and TryGet* methods use a boolean return value to indicate whether there was a value to get or not. It also plays relatively nicely with the NRT stuff... In my humble opinion, the current APIs are not a good design choice and since there is really only an ancient beta on the official nuget feed there is still plenty of room to maneuver and improve upon the API.

[jonsequitur]

An example of the same pattern can be found in IServiceProvider.GetService<T> and IServiceProvider.GetRequiredService<T>. You're right that the Try* pattern is absolutely more common, and therefore likely easier for more people to immediately understand. It benefits from being an older pattern in .NET.

I think it's a matter of personal preference though as to which of these patterns is "better." The Try* pattern results in a different code flow versus something that simply returns a value, and the inconsistency between the nullable versus non-nullable forms of the API can seem a bit arbitrary. That's just my personal opinion though.

You're still deep in beta. :-)

We're less deep in beta than we were when beta4 was published to nuget.org, and the steady improvements to the library over the last couple of years have wide adoption. We'd probably want to do an assessment of the impact of changing the behavior of GetValue. This might have been part of the reason for making a purely additive change with GetRequiredValue.