Skip to content

add GetRequiredValue to avoid null checks for required options #2564

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

Merged
merged 1 commit into from
May 29, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@
public System.Collections.Generic.IReadOnlyList<System.String> UnmatchedTokens { get; }
public System.CommandLine.Completions.CompletionContext GetCompletionContext()
public System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem> GetCompletions(System.Nullable<System.Int32> position = null)
public T GetRequiredValue<T>(Argument<T> argument)
public T GetRequiredValue<T>(Option<T> option)
public T GetRequiredValue<T>(System.String name)
public System.CommandLine.Parsing.ArgumentResult GetResult(Argument argument)
public System.CommandLine.Parsing.CommandResult GetResult(Command command)
public System.CommandLine.Parsing.OptionResult GetResult(Option option)
Expand Down Expand Up @@ -269,6 +272,9 @@ System.CommandLine.Parsing
public SymbolResult Parent { get; }
public System.Collections.Generic.IReadOnlyList<Token> Tokens { get; }
public System.Void AddError(System.String errorMessage)
public T GetRequiredValue<T>(Argument<T> argument)
public T GetRequiredValue<T>(Option<T> option)
public T GetRequiredValue<T>(System.String name)
public ArgumentResult GetResult(System.CommandLine.Argument argument)
public CommandResult GetResult(System.CommandLine.Command command)
public OptionResult GetResult(System.CommandLine.Option option)
Expand Down
22 changes: 20 additions & 2 deletions src/System.CommandLine.Tests/OptionTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,14 @@ public void When_options_use_different_prefixes_they_still_work(string prefix)
var result = rootCommand.Parse(prefix + "c value-for-c " + prefix + "a value-for-a");

result.GetValue(optionA).Should().Be("value-for-a");
result.GetRequiredValue(optionA).Should().Be("value-for-a");
result.GetRequiredValue<string>(optionA.Name).Should().Be("value-for-a");
result.GetResult(optionB).Should().BeNull();
result.Invoking(result => result.GetRequiredValue(optionB)).Should().Throw<InvalidOperationException>();
result.Invoking(result => result.GetRequiredValue<string>(optionB.Name)).Should().Throw<InvalidOperationException>();
result.GetValue(optionC).Should().Be("value-for-c");
result.GetRequiredValue(optionC).Should().Be("value-for-c");
result.GetRequiredValue<string>(optionC.Name).Should().Be("value-for-c");
}

[Fact]
Expand All @@ -243,12 +249,22 @@ public void Option_T_default_value_can_be_set_after_instantiation()
DefaultValueFactory = (_) => 123
};

new RootCommand { option }
var result = new RootCommand { option }
.Parse("")
.GetResult(option)
.GetResult(option);

result
.GetValueOrDefault<int>()
.Should()
.Be(123);

result.GetRequiredValue(option)
.Should()
.Be(123);

result.GetRequiredValue<int>(option.Name)
.Should()
.Be(123);
}

[Fact]
Expand Down Expand Up @@ -390,6 +406,8 @@ public void Multiple_identifier_token_instances_without_argument_tokens_can_be_p
var result = root.Parse("-v -v -v");

result.GetValue(option).Should().BeTrue();
result.GetRequiredValue(option).Should().BeTrue();
result.GetRequiredValue<bool>(option.Name).Should().BeTrue();
}

[Fact]
Expand Down
36 changes: 36 additions & 0 deletions src/System.CommandLine.Tests/ParserTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -826,6 +826,33 @@ public void Commands_can_have_default_argument_values()
GetValue(result, argument)
.Should()
.Be("default");

result.GetRequiredValue(argument)
.Should()
.Be("default");
}

[Fact]
public void GetRequiredValue_throws_when_argument_without_default_value_was_not_provided()
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we also testing Option.GetRequiredValue for these cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

ParserTests is a bit of a catchall. I think these would be better organized under ArgumentTests and OptionTests.

{
Argument<int> argument = new("the-arg");
Option<bool> option = new("--option");

Command command = new("command")
{
argument,
option
};

ParseResult result = command.Parse("command --option");

result.Invoking(result => result.GetRequiredValue(argument))
.Should()
.Throw<InvalidOperationException>();

result.Invoking(result => result.GetRequiredValue<int>(argument.Name))
.Should()
.Throw<InvalidOperationException>();
}

[Fact]
Expand Down Expand Up @@ -925,6 +952,11 @@ public void Command_default_argument_value_does_not_override_parsed_value()
.Name
.Should()
.Be("the-directory");

result.GetRequiredValue(argument)
.Name
.Should()
.Be("the-directory");
}

[Fact]
Expand Down Expand Up @@ -1160,6 +1192,10 @@ public void Arguments_can_match_subcommands()
GetValue(result, argument)
.Should()
.BeEquivalentSequenceTo("one", "two", "three", "subcommand", "four");

result.GetRequiredValue(argument)
.Should()
.BeEquivalentSequenceTo("one", "two", "three", "subcommand", "four");
}

[Theory]
Expand Down
29 changes: 29 additions & 0 deletions src/System.CommandLine/ParseResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,35 @@ CommandLineText is null
public T? GetValue<T>(string name)
=> RootCommandResult.GetValue<T>(name);

/// <summary>
/// Gets the parsed or default value for the specified required argument or throws.
/// </summary>
/// <param name="argument">The argument for which to get a value.</param>
/// <returns>The parsed value or a configured default.</returns>
/// <exception cref="InvalidOperationException">Thrown when required argument was not parsed or has no default value configured.</exception>
public T GetRequiredValue<T>(Argument<T> argument)
=> RootCommandResult.GetRequiredValue(argument);

/// <summary>
/// Gets the parsed or default value for the specified required option or throws.
/// </summary>
/// <param name="option">The option for which to get a value.</param>
/// <returns>The parsed value or a configured default.</returns>
/// <exception cref="InvalidOperationException">Thrown when required option was not parsed or has no default value configured.</exception>
public T GetRequiredValue<T>(Option<T> option)
=> RootCommandResult.GetRequiredValue(option);

/// <summary>
/// Gets the parsed or default value for the specified required symbol name, in the context of parsed command (not entire symbol tree).
/// </summary>
/// <param name="name">The name of the required Symbol for which to get a value.</param>
/// <returns>The parsed value or a configured default.</returns>
/// <exception cref="InvalidOperationException">Thrown when parsing resulted in parse error(s) or required symbol was not parsed or has no default value configured.</exception>
/// <exception cref="ArgumentException">Thrown when there was no symbol defined for given name for the parsed command.</exception>
/// <exception cref="InvalidCastException">Thrown when parsed result can not be cast to <typeparamref name="T"/>.</exception>
public T GetRequiredValue<T>(string name)
=> RootCommandResult.GetRequiredValue<T>(name);

/// <inheritdoc />
public override string ToString() => ParseDiagramAction.Diagram(this).ToString();

Expand Down
30 changes: 30 additions & 0 deletions src/System.CommandLine/Parsing/SymbolResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,22 @@ public IEnumerable<ParseError> Errors
return Argument<T>.CreateDefaultValue();
}

/// <inheritdoc cref="ParseResult.GetRequiredValue{T}(Argument{T})"/>
public T GetRequiredValue<T>(Argument<T> argument)
=> GetResult(argument) switch
{
ArgumentResult argumentResult => argumentResult.GetValueOrDefault<T>(),
null => throw new InvalidOperationException($"{argument.Name} is required but was not provided."),
};

/// <inheritdoc cref="ParseResult.GetRequiredValue{T}(Option{T})"/>
public T GetRequiredValue<T>(Option<T> option)
=> GetResult(option) switch
{
OptionResult optionResult => optionResult.GetValueOrDefault<T>(),
null => throw new InvalidOperationException($"{option.Name} is required but was not provided."),
};

/// <summary>
/// Gets the value for a symbol having the specified name anywhere in the parse tree.
/// </summary>
Expand All @@ -147,6 +163,20 @@ public IEnumerable<ParseError> Errors
return Argument<T>.CreateDefaultValue();
}

/// <summary>
/// Gets the value for a symbol having the specified name anywhere in the parse tree.
/// </summary>
/// <param name="name">The name of the symbol for which to find a result.</param>
/// <returns>An argument result if the argument was matched by the parser or has a default value; otherwise, <c>null</c>.</returns>
public T GetRequiredValue<T>(string name)
=> GetResult(name) switch
{
OptionResult optionResult => optionResult.GetValueOrDefault<T>(),
ArgumentResult argumentResult => argumentResult.GetValueOrDefault<T>(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this can still return null.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

My memory was that SymbolResult.GetResult would never return null but either I misremembered or that changed at some point. This looks great!

SymbolResult _ => throw new InvalidOperationException($"{name} is not an option or argument."),
_ => throw new InvalidOperationException($"{name} is required but was not provided."),
};

internal virtual bool UseDefaultValueFor(ArgumentResult argumentResult) => false;
}
}