Skip to content

more flexible error reporting for SymbolResult #2033

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.

Sign up for GitHub

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 15 commits into from
Feb 2, 2023
Merged

more flexible error reporting for SymbolResult #2033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
b0b488d
remove SymbolResult.ErrorMessage, use the new SymbolResult.ReportError
adamsitnik Jan 23, 2023
3006cb3
use all available information when creating error message for Argumen…
adamsitnik Jan 24, 2023
2ef226f
remove unused method
adamsitnik Jan 24, 2023
d8f92ef
last tweaks
adamsitnik Jan 24, 2023
f8370ae
address code review feedback:
adamsitnik Jan 25, 2023
66c3e4f
refactor the code: merge validation and default values creation into …
adamsitnik Jan 25, 2023
94f9705
refactor:
adamsitnik Jan 26, 2023
49ab482
fix a bug I have introduced: when no Tokens are available the result …
adamsitnik Jan 26, 2023
ecf99fd
re-implement OnlyTake, remove some virtual properties and get all the…
adamsitnik Jan 26, 2023
aa852d1
reduce the number of ctors and methods
adamsitnik Jan 26, 2023
bae82fe
solve the TODO: use Enum.TryParse when available
adamsitnik Jan 26, 2023
4f62218
with recent changes it's guaranteed that every OptionResult has corre…
adamsitnik Jan 27, 2023
172ffe4
rename the test to reflect what it does now
adamsitnik Jan 27, 2023
10fdeeb
Apply suggestions from code review
adamsitnik Jan 30, 2023
1307432
Address code review feedback
adamsitnik Jan 30, 2023
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
5 changes: 4 additions & 1 deletion ...ty.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,9 @@ System.CommandLine
public static LocalizationResources Instance { get; }
public System.String ArgumentConversionCannotParse(System.String value, System.Type expectedType)
public System.String ArgumentConversionCannotParseForCommand(System.String value, System.String commandAlias, System.Type expectedType)
public System.String ArgumentConversionCannotParseForCommand(System.String value, System.String commandAlias, System.Type expectedType, System.Collections.Generic.IEnumerable<System.String> completions)
public System.String ArgumentConversionCannotParseForOption(System.String value, System.String optionAlias, System.Type expectedType)
public System.String ArgumentConversionCannotParseForOption(System.String value, System.String optionAlias, System.Type expectedType, System.Collections.Generic.IEnumerable<System.String> completions)
public System.String DirectoryDoesNotExist(System.String path)
public System.String ErrorReadingResponseFile(System.String filePath, System.IO.IOException e)
public System.String ExceptionHandlerHeader()
Expand Down Expand Up @@ -386,6 +388,7 @@ System.CommandLine.IO
System.CommandLine.Parsing
public class ArgumentResult : SymbolResult
public System.CommandLine.Argument Argument { get; }
public System.Void AddError(System.String errorMessage)
public System.Object GetValueOrDefault()
public T GetValueOrDefault<T>()
public System.Void OnlyTake(System.Int32 numberOfTokens)
Expand Down Expand Up @@ -423,10 +426,10 @@ System.CommandLine.Parsing
public static System.Threading.Tasks.Task<System.Int32> InvokeAsync(this Parser parser, System.String[] args, System.CommandLine.IConsole console = null, System.Threading.CancellationToken cancellationToken = null)
public static System.CommandLine.ParseResult Parse(this Parser parser, System.String commandLine)
public abstract class SymbolResult
public System.String ErrorMessage { get; set; }
public System.CommandLine.LocalizationResources LocalizationResources { get; }
public SymbolResult Parent { get; }
public System.Collections.Generic.IReadOnlyList<Token> Tokens { get; }
public System.Void AddError(System.String errorMessage)
public ArgumentResult FindResultFor(System.CommandLine.Argument argument)
public CommandResult FindResultFor(System.CommandLine.Command command)
public OptionResult FindResultFor(System.CommandLine.Option option)
Expand Down
4 changes: 2 additions & 2 deletions src/System.CommandLine.NamingConventionBinder/ModelBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ private bool ShortCutTheBinding()
var valueSource = GetValueSource(bindingSources, bindingContext, ValueDescriptor, EnforceExplicitBinding);
return bindingContext.TryBindToScalarValue(ValueDescriptor,
valueSource,
bindingContext.ParseResult.CommandResult.LocalizationResources,
bindingContext.ParseResult,
out var boundValue)
? (true, boundValue?.Value, true)
: (false, null, false);
Expand Down Expand Up @@ -277,7 +277,7 @@ internal static (BoundValue? boundValue, bool usedNonDefault) GetBoundValue(
if (bindingContext.TryBindToScalarValue(
valueDescriptor,
valueSource,
bindingContext.ParseResult.CommandResult.LocalizationResources,
bindingContext.ParseResult,
out var boundValue))
{
return (boundValue, true);
Expand Down
14 changes: 7 additions & 7 deletions src/System.CommandLine.Tests/ArgumentTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public void Validation_failure_message_can_be_specified_when_parsing_tokens()
{
var argument = new Argument<FileSystemInfo>(result =>
{
result.ErrorMessage = "oops!";
result.AddError("oops!");
return null;
});

Expand All @@ -143,7 +143,7 @@ public void Validation_failure_message_can_be_specified_when_evaluating_default_
{
var argument = new Argument<FileSystemInfo>(result =>
{
result.ErrorMessage = "oops!";
result.AddError("oops!");
return null;
}, true);

Expand All @@ -164,7 +164,7 @@ public void Validation_failure_message_can_be_specified_when_evaluating_default_
"-x",
result =>
{
result.ErrorMessage = "oops!";
result.AddError("oops!");
return null;
}, true);

Expand Down Expand Up @@ -389,15 +389,15 @@ public void Multiple_command_arguments_can_have_custom_parse_delegates()
{
new Argument<FileInfo[]>("from", argumentResult =>
{
argumentResult.ErrorMessage = "nope";
argumentResult.AddError("nope");
return null;
}, true)
{
Arity = new ArgumentArity(0, 2)
},
new Argument<DirectoryInfo>("to", argumentResult =>
{
argumentResult.ErrorMessage = "UH UH";
argumentResult.AddError("UH UH");
return null;
}, true)
{
Expand Down Expand Up @@ -426,7 +426,7 @@ public void When_custom_conversion_fails_then_an_option_does_not_accept_further_
new Argument<string>(),
new Option<string>("-x", argResult =>
{
argResult.ErrorMessage = "nope";
argResult.AddError("nope");
return default;
})
};
Expand All @@ -446,7 +446,7 @@ public void When_argument_cannot_be_parsed_as_the_specified_type_then_getting_va
return value;
}

argumentResult.ErrorMessage = $"'{argumentResult.Tokens.Single().Value}' is not an integer";
argumentResult.AddError($"'{argumentResult.Tokens.Single().Value}' is not an integer");

return default;
});
Expand Down
4 changes: 2 additions & 2 deletions src/System.CommandLine.Tests/OptionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,11 +287,11 @@ public void Option_T_default_value_is_validated()
{
var option = new Option<int>("-x", () => 123);
option.Validators.Add(symbol =>
symbol.ErrorMessage = symbol.Tokens
symbol.AddError(symbol.Tokens
.Select(t => t.Value)
.Where(v => v == "123")
.Select(_ => "ERR")
.FirstOrDefault());
.First()));

option
.Parse("-x 123")
Expand Down
2 changes: 1 addition & 1 deletion src/System.CommandLine.Tests/ParseDiagramTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public void Parse_diagram_shows_type_conversion_errors()

result.Diagram()
.Should()
.Be($"[ {RootCommand.ExecutableName} [ -f !<not-an-int> ] ]");
.Be($"[ {RootCommand.ExecutableName} ![ -f <not-an-int> ] ]");
}

[Fact]
Expand Down
76 changes: 44 additions & 32 deletions src/System.CommandLine.Tests/ParsingValidationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,17 @@ public void When_FromAmong_is_used_then_the_OptionResult_ErrorMessage_is_set()

var parseResult = command.Parse("test --opt c");

parseResult.FindResultFor(option)
.ErrorMessage
.Should()
.Be(parseResult.Errors.Single().Message)
.And
.Should()
.NotBeNull();
var error = parseResult.Errors.Single();

error
.Message
.Should()
.Be(parseResult.CommandResult.LocalizationResources.UnrecognizedArgument("c", new []{ "a", "b"}));
error
.SymbolResult
.Should()
.BeOfType<OptionResult>();

}

[Fact] // https://github.com/dotnet/command-line-api/issues/1475
Expand All @@ -79,13 +83,16 @@ public void When_FromAmong_is_used_then_the_ArgumentResult_ErrorMessage_is_set()

var parseResult = command.Parse("test c");

parseResult.FindResultFor(argument)
.ErrorMessage
.Should()
.Be(parseResult.Errors.Single().Message)
.And
.Should()
.NotBeNull();
var error = parseResult.Errors.Single();

error
.Message
.Should()
.Be(parseResult.CommandResult.LocalizationResources.UnrecognizedArgument("c", new []{ "a", "b"}));
error
.SymbolResult
.Should()
.BeOfType<ArgumentResult>();
}

[Fact] // https://github.com/dotnet/command-line-api/issues/1556
Expand Down Expand Up @@ -171,7 +178,7 @@ public void When_FromAmong_is_used_for_multiple_arguments_and_invalid_input_is_p
}

[Fact]
public void When_FromAmong_is_used_and_multiple_invalid_inputs_are_provided_the_error_mentions_first_invalid_argument()
public void When_FromAmong_is_used_and_multiple_invalid_inputs_are_provided_the_errors_mention_all_invalid_arguments()
{
Option<string[]> option = new(new[] { "--columns" });
option.AcceptOnlyFromAmong("author", "language", "tags", "type");
Expand All @@ -185,12 +192,17 @@ public void When_FromAmong_is_used_and_multiple_invalid_inputs_are_provided_the_

var result = command.Parse("list --columns c1 c2");

// Currently there is no possibility for a single validator to produce multiple errors,
// so only the first one is checked.
result.Errors.Count.Should().Be(2);

result.Errors[0]
.Message
.Should()
.Be(LocalizationResources.Instance.UnrecognizedArgument("c1", new[] { "author", "language", "tags", "type" }));
.Message
.Should()
.Be(LocalizationResources.Instance.UnrecognizedArgument("c1", new[] { "author", "language", "tags", "type" }));

result.Errors[1]
.Message
.Should()
.Be(LocalizationResources.Instance.UnrecognizedArgument("c2", new[] { "author", "language", "tags", "type" }));
}

[Fact]
Expand Down Expand Up @@ -334,7 +346,7 @@ public void A_custom_validator_can_be_added_to_a_command()
if (commandResult.Children.Any(sr => ((OptionResult)sr).Option.HasAlias("--one")) &&
commandResult.Children.Any(sr => ((OptionResult)sr).Option.HasAlias("--two")))
{
commandResult.ErrorMessage = "Options '--one' and '--two' cannot be used together.";
commandResult.AddError("Options '--one' and '--two' cannot be used together.");
}
});

Expand All @@ -358,7 +370,7 @@ public void A_custom_validator_can_be_added_to_an_option()
{
var value = r.GetValueOrDefault<int>();

r.ErrorMessage = $"Option {r.Token.Value} cannot be set to {value}";
r.AddError($"Option {r.Token.Value} cannot be set to {value}");
});

var command = new RootCommand { option };
Expand All @@ -385,7 +397,7 @@ public void A_custom_validator_can_be_added_to_an_argument()
{
var value = r.GetValueOrDefault<int>();

r.ErrorMessage = $"Argument {r.Argument.Name} cannot be set to {value}";
r.AddError($"Argument {r.Argument.Name} cannot be set to {value}");
});

var command = new RootCommand { argument };
Expand Down Expand Up @@ -449,7 +461,7 @@ public void Validators_on_global_options_are_executed_when_invoking_a_subcommand
var option = new Option<FileInfo>("--file");
option.Validators.Add(r =>
{
r.ErrorMessage = "Invoked validator";
r.AddError("Invoked validator");
});

var subCommand = new Command("subcommand");
Expand Down Expand Up @@ -484,7 +496,7 @@ public async Task A_custom_validator_added_to_a_global_option_is_checked(string
var handlerWasCalled = false;

var globalOption = new Option<int>("--value");
globalOption.Validators.Add(r => r.ErrorMessage = "oops!");
globalOption.Validators.Add(r => r.AddError("oops!"));

var grandchildCommand = new Command("grandchild");

Expand Down Expand Up @@ -514,7 +526,7 @@ public void Custom_validator_error_messages_are_not_repeated()
{
var errorMessage = "that's not right...";
var argument = new Argument<string>();
argument.Validators.Add(r => r.ErrorMessage = errorMessage);
argument.Validators.Add(r => r.AddError(errorMessage));

var cmd = new Command("get")
{
Expand All @@ -541,7 +553,7 @@ public void The_parsed_value_of_an_argument_is_available_within_a_validator()

if (value < 0 || value > 100)
{
result.ErrorMessage = errorMessage;
result.AddError(errorMessage);
}
});

Expand All @@ -565,7 +577,7 @@ public void The_parsed_value_of_an_option_is_available_within_a_validator()

if (value < 0 || value > 100)
{
result.ErrorMessage = errorMessage;
result.AddError(errorMessage);
}
});

Expand Down Expand Up @@ -1196,7 +1208,7 @@ public void Arity_failures_are_not_reported_for_both_an_argument_and_its_parent_
public void Multiple_validators_on_the_same_command_do_not_report_duplicate_errors()
{
var command = new RootCommand();
command.Validators.Add(result => result.ErrorMessage = "Wrong");
command.Validators.Add(result => result.AddError("Wrong"));
command.Validators.Add(_ => { });

var parseResult = command.Parse("");
Expand All @@ -1214,7 +1226,7 @@ public void Multiple_validators_on_the_same_command_do_not_report_duplicate_erro
public void Multiple_validators_on_the_same_option_do_not_report_duplicate_errors()
{
var option = new Option<string>("-x");
option.Validators.Add(result => result.ErrorMessage = "Wrong");
option.Validators.Add(result => result.AddError("Wrong"));
option.Validators.Add(_ => { });

var command = new RootCommand
Expand All @@ -1237,7 +1249,7 @@ public void Multiple_validators_on_the_same_option_do_not_report_duplicate_error
public void Multiple_validators_on_the_same_argument_do_not_report_duplicate_errors()
{
var argument = new Argument<string>();
argument.Validators.Add(result => result.ErrorMessage = "Wrong");
argument.Validators.Add(result => result.AddError("Wrong"));
argument.Validators.Add(_ => { });

var command = new RootCommand
Expand All @@ -1262,7 +1274,7 @@ internal void When_there_is_an_arity_error_then_further_errors_are_not_reported(
var option = new Option<string>("-o");
option.Validators.Add(result =>
{
result.ErrorMessage = "OOPS";
result.AddError("OOPS");
}); //all good;

var command = new Command("comm")
Expand Down
2 changes: 2 additions & 0 deletions src/System.CommandLine/Argument.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ private protected override string DefaultName
/// </summary>
public List<Action<ArgumentResult>> Validators => _validators ??= new ();

internal bool HasValidators => (_validators?.Count ?? 0) > 0;

/// <summary>
/// Gets the default value for the argument.
/// </summary>
Expand Down
Loading