add GetRequiredValue to avoid null checks for required options #2564
Conversation
|
cc @jaredpar - weren't we jsut talking about some nullability analysis for this use method? |
| } | ||
|
|
||
| [Fact] | ||
| public void GetRequiredValue_throws_when_argument_without_default_value_was_not_provided() |
There was a problem hiding this comment.
Are we also testing Option.GetRequiredValue for these cases?
| } | ||
|
|
||
| [Fact] | ||
| public void GetRequiredValue_throws_when_argument_without_default_value_was_not_provided() |
There was a problem hiding this comment.
ParserTests is a bit of a catchall. I think these would be better organized under ArgumentTests and OptionTests.
| => GetResult(name) switch | ||
| { | ||
| OptionResult optionResult => optionResult.GetValueOrDefault<T>(), | ||
| ArgumentResult argumentResult => argumentResult.GetValueOrDefault<T>(), |
There was a problem hiding this comment.
I believe this can still return null.
There was a problem hiding this comment.
I believe this can still return
null.
The only scenario I can come up with is the user defining null as the default value via DefaultValueFactory. Do you believe we should throw in such cases?
There was a problem hiding this comment.
My memory was that SymbolResult.GetResult would never return null but either I misremembered or that changed at some point. This looks great!
Yes. It's the number one cause of nullable suppressions in sdk after I changed the opt in model. |
The existing
GetValuemethod returns nullableT?, which often leads to plenty of nullability warnings when working with required options.GetRequiredValuereturns the parsed (or configured default value) or throws.Option<FileInfo> fileOption = new("--file") { Required = true }; RootCommand command = new() { fileOption }; command.SetAction(paseResult => { - FileInfo file = paseResult.GetValue(fileOption)!; // mind the ! + FileInfo file = paseResult.GetRequiredValue(fileOption); Console.WriteLine($"File name: {file.FullName}"); });