Skip to content

Commit 506cb2f

Browse files
adamsitnikjonsequitur
adamsitnik
and
jonsequitur
authored
more flexible error reporting for SymbolResult (#2033)
* remove SymbolResult.ErrorMessage, use the new SymbolResult.AddError * use all available information when creating error message for ArgumentConversionResult * test coverage improvement (multiple errors for one result) * refactor: * merge validation and default values creation into one (20% perf gain on simple scenarios) * move CommandResult validation logic to CommandResult type * simplify command handler validation * don't search for ArgumentResult when we know if up front * re-implement OnlyTake, remove some virtual properties and get all the tests passing again * reduce the number of ctors and methods * remove null checks as it's internal API and we are covered by nullable annotations * solve the TODO: use Enum.TryParse when available * with recent changes it's guaranteed that every OptionResult has corresponding ArgumentResult Co-authored-by: Jon Sequeira <[email protected]>
1 parent 6524142 commit 506cb2f

39 files changed

+680
-772
lines changed

‎src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,9 @@ System.CommandLine
146146
public static LocalizationResources Instance { get; }
147147
public System.String ArgumentConversionCannotParse(System.String value, System.Type expectedType)
148148
public System.String ArgumentConversionCannotParseForCommand(System.String value, System.String commandAlias, System.Type expectedType)
149+
public System.String ArgumentConversionCannotParseForCommand(System.String value, System.String commandAlias, System.Type expectedType, System.Collections.Generic.IEnumerable<System.String> completions)
149150
public System.String ArgumentConversionCannotParseForOption(System.String value, System.String optionAlias, System.Type expectedType)
151+
public System.String ArgumentConversionCannotParseForOption(System.String value, System.String optionAlias, System.Type expectedType, System.Collections.Generic.IEnumerable<System.String> completions)
150152
public System.String DirectoryDoesNotExist(System.String path)
151153
public System.String ErrorReadingResponseFile(System.String filePath, System.IO.IOException e)
152154
public System.String ExceptionHandlerHeader()
@@ -386,6 +388,7 @@ System.CommandLine.IO
386388
System.CommandLine.Parsing
387389
public class ArgumentResult : SymbolResult
388390
public System.CommandLine.Argument Argument { get; }
391+
public System.Void AddError(System.String errorMessage)
389392
public System.Object GetValueOrDefault()
390393
public T GetValueOrDefault<T>()
391394
public System.Void OnlyTake(System.Int32 numberOfTokens)
@@ -423,10 +426,10 @@ System.CommandLine.Parsing
423426
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)
424427
public static System.CommandLine.ParseResult Parse(this Parser parser, System.String commandLine)
425428
public abstract class SymbolResult
426-
public System.String ErrorMessage { get; set; }
427429
public System.CommandLine.LocalizationResources LocalizationResources { get; }
428430
public SymbolResult Parent { get; }
429431
public System.Collections.Generic.IReadOnlyList<Token> Tokens { get; }
432+
public System.Void AddError(System.String errorMessage)
430433
public ArgumentResult FindResultFor(System.CommandLine.Argument argument)
431434
public CommandResult FindResultFor(System.CommandLine.Command command)
432435
public OptionResult FindResultFor(System.CommandLine.Option option)

‎src/System.CommandLine.NamingConventionBinder/ModelBinder.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ private bool ShortCutTheBinding()
132132
var valueSource = GetValueSource(bindingSources, bindingContext, ValueDescriptor, EnforceExplicitBinding);
133133
return bindingContext.TryBindToScalarValue(ValueDescriptor,
134134
valueSource,
135-
bindingContext.ParseResult.CommandResult.LocalizationResources,
135+
bindingContext.ParseResult,
136136
out var boundValue)
137137
? (true, boundValue?.Value, true)
138138
: (false, null, false);
@@ -277,7 +277,7 @@ internal static (BoundValue? boundValue, bool usedNonDefault) GetBoundValue(
277277
if (bindingContext.TryBindToScalarValue(
278278
valueDescriptor,
279279
valueSource,
280-
bindingContext.ParseResult.CommandResult.LocalizationResources,
280+
bindingContext.ParseResult,
281281
out var boundValue))
282282
{
283283
return (boundValue, true);

‎src/System.CommandLine.Tests/ArgumentTests.cs‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ public void Validation_failure_message_can_be_specified_when_parsing_tokens()
124124
{
125125
var argument = new Argument<FileSystemInfo>(result =>
126126
{
127-
result.ErrorMessage = "oops!";
127+
result.AddError("oops!");
128128
return null;
129129
});
130130

@@ -143,7 +143,7 @@ public void Validation_failure_message_can_be_specified_when_evaluating_default_
143143
{
144144
var argument = new Argument<FileSystemInfo>(result =>
145145
{
146-
result.ErrorMessage = "oops!";
146+
result.AddError("oops!");
147147
return null;
148148
}, true);
149149

@@ -164,7 +164,7 @@ public void Validation_failure_message_can_be_specified_when_evaluating_default_
164164
"-x",
165165
result =>
166166
{
167-
result.ErrorMessage = "oops!";
167+
result.AddError("oops!");
168168
return null;
169169
}, true);
170170

@@ -389,15 +389,15 @@ public void Multiple_command_arguments_can_have_custom_parse_delegates()
389389
{
390390
new Argument<FileInfo[]>("from", argumentResult =>
391391
{
392-
argumentResult.ErrorMessage = "nope";
392+
argumentResult.AddError("nope");
393393
return null;
394394
}, true)
395395
{
396396
Arity = new ArgumentArity(0, 2)
397397
},
398398
new Argument<DirectoryInfo>("to", argumentResult =>
399399
{
400-
argumentResult.ErrorMessage = "UH UH";
400+
argumentResult.AddError("UH UH");
401401
return null;
402402
}, true)
403403
{
@@ -426,7 +426,7 @@ public void When_custom_conversion_fails_then_an_option_does_not_accept_further_
426426
new Argument<string>(),
427427
new Option<string>("-x", argResult =>
428428
{
429-
argResult.ErrorMessage = "nope";
429+
argResult.AddError("nope");
430430
return default;
431431
})
432432
};
@@ -446,7 +446,7 @@ public void When_argument_cannot_be_parsed_as_the_specified_type_then_getting_va
446446
return value;
447447
}
448448

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

451451
return default;
452452
});

‎src/System.CommandLine.Tests/OptionTests.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -287,11 +287,11 @@ public void Option_T_default_value_is_validated()
287287
{
288288
var option = new Option<int>("-x", () => 123);
289289
option.Validators.Add(symbol =>
290-
symbol.ErrorMessage = symbol.Tokens
290+
symbol.AddError(symbol.Tokens
291291
.Select(t => t.Value)
292292
.Where(v => v == "123")
293293
.Select(_ => "ERR")
294-
.FirstOrDefault());
294+
.First()));
295295

296296
option
297297
.Parse("-x 123")

‎src/System.CommandLine.Tests/ParseDiagramTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public void Parse_diagram_shows_type_conversion_errors()
5959

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

6565
[Fact]

‎src/System.CommandLine.Tests/ParsingValidationTests.cs‎

Lines changed: 44 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -60,13 +60,17 @@ public void When_FromAmong_is_used_then_the_OptionResult_ErrorMessage_is_set()
6060

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

63-
parseResult.FindResultFor(option)
64-
.ErrorMessage
65-
.Should()
66-
.Be(parseResult.Errors.Single().Message)
67-
.And
68-
.Should()
69-
.NotBeNull();
63+
var error = parseResult.Errors.Single();
64+
65+
error
66+
.Message
67+
.Should()
68+
.Be(parseResult.CommandResult.LocalizationResources.UnrecognizedArgument("c", new []{ "a", "b"}));
69+
error
70+
.SymbolResult
71+
.Should()
72+
.BeOfType<OptionResult>();
73+
7074
}
7175

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

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

82-
parseResult.FindResultFor(argument)
83-
.ErrorMessage
84-
.Should()
85-
.Be(parseResult.Errors.Single().Message)
86-
.And
87-
.Should()
88-
.NotBeNull();
86+
var error = parseResult.Errors.Single();
87+
88+
error
89+
.Message
90+
.Should()
91+
.Be(parseResult.CommandResult.LocalizationResources.UnrecognizedArgument("c", new []{ "a", "b"}));
92+
error
93+
.SymbolResult
94+
.Should()
95+
.BeOfType<ArgumentResult>();
8996
}
9097

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

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

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

188-
// Currently there is no possibility for a single validator to produce multiple errors,
189-
// so only the first one is checked.
195+
result.Errors.Count.Should().Be(2);
196+
190197
result.Errors[0]
191-
.Message
192-
.Should()
193-
.Be(LocalizationResources.Instance.UnrecognizedArgument("c1", new[] { "author", "language", "tags", "type" }));
198+
.Message
199+
.Should()
200+
.Be(LocalizationResources.Instance.UnrecognizedArgument("c1", new[] { "author", "language", "tags", "type" }));
201+
202+
result.Errors[1]
203+
.Message
204+
.Should()
205+
.Be(LocalizationResources.Instance.UnrecognizedArgument("c2", new[] { "author", "language", "tags", "type" }));
194206
}
195207

196208
[Fact]
@@ -334,7 +346,7 @@ public void A_custom_validator_can_be_added_to_a_command()
334346
if (commandResult.Children.Any(sr => ((OptionResult)sr).Option.HasAlias("--one")) &&
335347
commandResult.Children.Any(sr => ((OptionResult)sr).Option.HasAlias("--two")))
336348
{
337-
commandResult.ErrorMessage = "Options '--one' and '--two' cannot be used together.";
349+
commandResult.AddError("Options '--one' and '--two' cannot be used together.");
338350
}
339351
});
340352

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

361-
r.ErrorMessage = $"Option {r.Token.Value} cannot be set to {value}";
373+
r.AddError($"Option {r.Token.Value} cannot be set to {value}");
362374
});
363375

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

388-
r.ErrorMessage = $"Argument {r.Argument.Name} cannot be set to {value}";
400+
r.AddError($"Argument {r.Argument.Name} cannot be set to {value}");
389401
});
390402

391403
var command = new RootCommand { argument };
@@ -449,7 +461,7 @@ public void Validators_on_global_options_are_executed_when_invoking_a_subcommand
449461
var option = new Option<FileInfo>("--file");
450462
option.Validators.Add(r =>
451463
{
452-
r.ErrorMessage = "Invoked validator";
464+
r.AddError("Invoked validator");
453465
});
454466

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

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

489501
var grandchildCommand = new Command("grandchild");
490502

@@ -514,7 +526,7 @@ public void Custom_validator_error_messages_are_not_repeated()
514526
{
515527
var errorMessage = "that's not right...";
516528
var argument = new Argument<string>();
517-
argument.Validators.Add(r => r.ErrorMessage = errorMessage);
529+
argument.Validators.Add(r => r.AddError(errorMessage));
518530

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

542554
if (value < 0 || value > 100)
543555
{
544-
result.ErrorMessage = errorMessage;
556+
result.AddError(errorMessage);
545557
}
546558
});
547559

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

566578
if (value < 0 || value > 100)
567579
{
568-
result.ErrorMessage = errorMessage;
580+
result.AddError(errorMessage);
569581
}
570582
});
571583

@@ -1196,7 +1208,7 @@ public void Arity_failures_are_not_reported_for_both_an_argument_and_its_parent_
11961208
public void Multiple_validators_on_the_same_command_do_not_report_duplicate_errors()
11971209
{
11981210
var command = new RootCommand();
1199-
command.Validators.Add(result => result.ErrorMessage = "Wrong");
1211+
command.Validators.Add(result => result.AddError("Wrong"));
12001212
command.Validators.Add(_ => { });
12011213

12021214
var parseResult = command.Parse("");
@@ -1214,7 +1226,7 @@ public void Multiple_validators_on_the_same_command_do_not_report_duplicate_erro
12141226
public void Multiple_validators_on_the_same_option_do_not_report_duplicate_errors()
12151227
{
12161228
var option = new Option<string>("-x");
1217-
option.Validators.Add(result => result.ErrorMessage = "Wrong");
1229+
option.Validators.Add(result => result.AddError("Wrong"));
12181230
option.Validators.Add(_ => { });
12191231

12201232
var command = new RootCommand
@@ -1237,7 +1249,7 @@ public void Multiple_validators_on_the_same_option_do_not_report_duplicate_error
12371249
public void Multiple_validators_on_the_same_argument_do_not_report_duplicate_errors()
12381250
{
12391251
var argument = new Argument<string>();
1240-
argument.Validators.Add(result => result.ErrorMessage = "Wrong");
1252+
argument.Validators.Add(result => result.AddError("Wrong"));
12411253
argument.Validators.Add(_ => { });
12421254

12431255
var command = new RootCommand
@@ -1262,7 +1274,7 @@ internal void When_there_is_an_arity_error_then_further_errors_are_not_reported(
12621274
var option = new Option<string>("-o");
12631275
option.Validators.Add(result =>
12641276
{
1265-
result.ErrorMessage = "OOPS";
1277+
result.AddError("OOPS");
12661278
}); //all good;
12671279

12681280
var command = new Command("comm")

‎src/System.CommandLine/Argument.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,8 @@ private protected override string DefaultName
107107
/// </summary>
108108
public List<Action<ArgumentResult>> Validators => _validators ??= new ();
109109

110+
internal bool HasValidators => (_validators?.Count ?? 0) > 0;
111+
110112
/// <summary>
111113
/// Gets the default value for the argument.
112114
/// </summary>

0 commit comments

Comments
 (0)