Skip to content
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

Powderhouse - Fix 2392 Error Message #2415

Open
wants to merge 1 commit into
base: main-powderhouse
Choose a base branch
from
Open

Powderhouse - Fix 2392 Error Message #2415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 16 additions & 1 deletion src/System.CommandLine.Tests/ParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Collections.Generic;
Expand Down Expand Up @@ -1626,6 +1626,21 @@ public void When_option_arguments_are_greater_than_maximum_arity_then_an_error_i
.Contain(LocalizationResources.UnrecognizedCommandOrArgument("4"));
}

[Fact]
public void When_the_number_of_option_arguments_are_greater_than_maximum_arity_then_an_error_is_returned()
{
var command = new CliCommand("the-command")
{
new CliOption<int[]>("-x") { Arity = new ArgumentArity(2, 3)}
};

CliParser.Parse(command, "-x 1 -x 2 -x 3 -x 4")
.Errors
.Select(e => e.Message)
.Should()
.Contain(LocalizationResources.OptionArgumentsMaximumExceeded("-x", 3, 4));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use Single here as the state we are testing should be a single error.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this one we are not testing for a single error message. It is testing that the new error message is being used. In this case the setup of this test is setup to match the others in this test.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can table this because I think we have other things to focus on, I strongly think we are testing a single state with a limited focus and that limiting the confirmation sets us up for false positives.

The definition of single state with a limited focus can vary, which is of course why we disagree on this. This is a question I'd like the .NET libraries team to weigh in on, since they'll be working with this long term.

Copy link
Member

Choose a reason for hiding this comment

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

There are a couple ways to approach this kind of test that would be aligned with what I've seen in the dotnet/runtime test practices.

  1. There should be tests that validate the expected errors are present
  2. There should be tests that validate no unexpected errors can pop up in the results

Those can be achieved together or separately. The approaches I've seen used in dotnet/runtime depend on how much the tests are adhering to DRY. If the tests follow DRY patterns and factor boilerplate away such that test scenarios can be reused across several tests, and it's therefore desired for the set of results to start to include more entries, then the test would just check that the expected result is one of the items in the set. We guard against unexpected entries in another way. But the default approach we take is for each test to validate the results match the entire set of expected results. Sequence equality is the preferred approach for that when feasible, but styles vary on this and you'll even see dotnet/runtime tests that perform asserts in a loop over the set.

One of the factors we keep in mind for dotnet/runtime is that there is overhead in discovering tests, and with the body of tests we have and the configurations we run them against, we favor multi-assert tests more than I personally would typically endorse--but extremely granular (pure) tests add time and money costs to every CI run.

}

// TODO: Tests tokens which is no longer exposed, and should be replaced with equivalent test using ParseResult
/*
[Fact]
Expand Down
19 changes: 14 additions & 5 deletions src/System.CommandLine/ArgumentArity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,20 @@ internal static bool Validate(ArgumentResult argumentResult, [NotNullWhen(false)
{
if (!optionResult.Option.AllowMultipleArgumentsPerToken)
{
error = ArgumentConversionResult.Failure(
argumentResult,
LocalizationResources.ExpectsOneArgument(optionResult),
ArgumentConversionResultType.FailedTooManyArguments);

if (argumentResult.Argument.Arity.MaximumNumberOfValues > 1)
{
error = ArgumentConversionResult.Failure(
argumentResult,
LocalizationResources.OptionArgumentsMaximumExceeded(optionResult, argumentResult.Argument.Arity.MaximumNumberOfValues),
ArgumentConversionResultType.FailedTooManyArguments);
}
else
{
error = ArgumentConversionResult.Failure(
argumentResult,
LocalizationResources.ExpectsOneArgument(optionResult),
ArgumentConversionResultType.FailedTooManyArguments);
}
return false;
}
}
Expand Down
15 changes: 14 additions & 1 deletion src/System.CommandLine/LocalizationResources.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,23 @@ namespace System.CommandLine
internal static class LocalizationResources
{
/// <summary>
/// Interpolates values into a localized string similar to Command &apos;{0}&apos; expects a single argument but {1} were provided.
/// Interpolates values into a localized string similar to Option &apos;{0}&apos; expects a single argument but {1} were provided.
/// </summary>
internal static string ExpectsOneArgument(OptionResult optionResult)
=> GetResourceString(Properties.Resources.OptionExpectsOneArgument, GetOptionName(optionResult), optionResult.Tokens.Count);

/// <summary>
/// Interpolates values into a localized string similar to Option &apos;{0}&apos; expects at most {1} arguments but {2} were provided.
/// </summary>
internal static string OptionArgumentsMaximumExceeded(OptionResult optionResult, int maximumOptionArgumentsAllowed)
=> OptionArgumentsMaximumExceeded(GetOptionName(optionResult), maximumOptionArgumentsAllowed, optionResult.Tokens.Count);

/// <summary>
/// Interpolates values into a localized string similar to Option &apos;{0}&apos; expects at most {1} arguments but {2} were provided.
/// </summary>
internal static string OptionArgumentsMaximumExceeded(string optionName, int maximumOptionArgumentsAllowed, int foundTokenCount)
=> GetResourceString(Properties.Resources.OptionArgumentsMaximumExceeded, optionName, maximumOptionArgumentsAllowed, foundTokenCount);

/*
/// <summary>
/// Interpolates values into a localized string similar to Directory does not exist: {0}.
Expand Down
9 changes: 9 additions & 0 deletions src/System.CommandLine/Properties/Resources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/System.CommandLine/Properties/Resources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,4 +225,7 @@
<data name="ArgumentConversionCannotParseForOption_Completions" xml:space="preserve">
<value>Cannot parse argument '{0}' for option '{1}' as expected type '{2}'. Did you mean one of the following?{3}</value>
</data>
<data name="OptionArgumentsMaximumExceeded" xml:space="preserve">
<value>Option '{0}' expects at most {1} arguments but {2} were provided.</value>
</data>
</root>
5 changes: 5 additions & 0 deletions src/System.CommandLine/Properties/xlf/Resources.cs.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@
<target state="translated">Znak se v cestě nepovoluje: '{0}'.</target>
<note />
</trans-unit>
<trans-unit id="OptionArgumentsMaximumExceeded">
<source>Option '{0}' expects at most {1} arguments but {2} were provided.</source>
<target state="new">Option '{0}' expects at most {1} arguments but {2} were provided.</target>
<note />
</trans-unit>
<trans-unit id="OptionExpectsOneArgument">
<source>Option '{0}' expects a single argument but {1} were provided.</source>
<target state="new">Option '{0}' expects a single argument but {1} were provided.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/System.CommandLine/Properties/xlf/Resources.de.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@
<target state="translated">Zeichen in Pfad nicht zulässig: '{0}'.</target>
<note />
</trans-unit>
<trans-unit id="OptionArgumentsMaximumExceeded">
<source>Option '{0}' expects at most {1} arguments but {2} were provided.</source>
<target state="new">Option '{0}' expects at most {1} arguments but {2} were provided.</target>
<note />
</trans-unit>
<trans-unit id="OptionExpectsOneArgument">
<source>Option '{0}' expects a single argument but {1} were provided.</source>
<target state="new">Option '{0}' expects a single argument but {1} were provided.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/System.CommandLine/Properties/xlf/Resources.es.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@
<target state="translated">Carácter no permitido en una ruta: '{0}'.</target>
<note></note>
</trans-unit>
<trans-unit id="OptionArgumentsMaximumExceeded">
<source>Option '{0}' expects at most {1} arguments but {2} were provided.</source>
<target state="new">Option '{0}' expects at most {1} arguments but {2} were provided.</target>
<note />
</trans-unit>
<trans-unit id="OptionExpectsOneArgument">
<source>Option '{0}' expects a single argument but {1} were provided.</source>
<target state="translated">La opción '{0}' espera un solo argumento, pero se proporcionaron {1}.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/System.CommandLine/Properties/xlf/Resources.fr.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@
<target state="translated">Caractère non autorisé dans un chemin : '{0}'.</target>
<note />
</trans-unit>
<trans-unit id="OptionArgumentsMaximumExceeded">
<source>Option '{0}' expects at most {1} arguments but {2} were provided.</source>
<target state="new">Option '{0}' expects at most {1} arguments but {2} were provided.</target>
<note />
</trans-unit>
<trans-unit id="OptionExpectsOneArgument">
<source>Option '{0}' expects a single argument but {1} were provided.</source>
<target state="new">Option '{0}' expects a single argument but {1} were provided.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/System.CommandLine/Properties/xlf/Resources.it.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@
<target state="translated">Il carattere non è consentito in un percorso: '{0}'.</target>
<note />
</trans-unit>
<trans-unit id="OptionArgumentsMaximumExceeded">
<source>Option '{0}' expects at most {1} arguments but {2} were provided.</source>
<target state="new">Option '{0}' expects at most {1} arguments but {2} were provided.</target>
<note />
</trans-unit>
<trans-unit id="OptionExpectsOneArgument">
<source>Option '{0}' expects a single argument but {1} were provided.</source>
<target state="new">Option '{0}' expects a single argument but {1} were provided.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/System.CommandLine/Properties/xlf/Resources.ja.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@
<target state="translated">パスで使用することが許可されていない文字: '{0}'.</target>
<note />
</trans-unit>
<trans-unit id="OptionArgumentsMaximumExceeded">
<source>Option '{0}' expects at most {1} arguments but {2} were provided.</source>
<target state="new">Option '{0}' expects at most {1} arguments but {2} were provided.</target>
<note />
</trans-unit>
<trans-unit id="OptionExpectsOneArgument">
<source>Option '{0}' expects a single argument but {1} were provided.</source>
<target state="new">Option '{0}' expects a single argument but {1} were provided.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/System.CommandLine/Properties/xlf/Resources.ko.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@
<target state="translated">경로에 사용할 수 없는 문자: '{0}'.</target>
<note />
</trans-unit>
<trans-unit id="OptionArgumentsMaximumExceeded">
<source>Option '{0}' expects at most {1} arguments but {2} were provided.</source>
<target state="new">Option '{0}' expects at most {1} arguments but {2} were provided.</target>
<note />
</trans-unit>
<trans-unit id="OptionExpectsOneArgument">
<source>Option '{0}' expects a single argument but {1} were provided.</source>
<target state="new">Option '{0}' expects a single argument but {1} were provided.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/System.CommandLine/Properties/xlf/Resources.pl.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@
<target state="translated">Znak jest niedozwolony w ścieżce: '{0}'.</target>
<note />
</trans-unit>
<trans-unit id="OptionArgumentsMaximumExceeded">
<source>Option '{0}' expects at most {1} arguments but {2} were provided.</source>
<target state="new">Option '{0}' expects at most {1} arguments but {2} were provided.</target>
<note />
</trans-unit>
<trans-unit id="OptionExpectsOneArgument">
<source>Option '{0}' expects a single argument but {1} were provided.</source>
<target state="new">Option '{0}' expects a single argument but {1} were provided.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/System.CommandLine/Properties/xlf/Resources.pt-BR.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@
<target state="translated">Caractere não permitido em um caminho: '{0}'.</target>
<note />
</trans-unit>
<trans-unit id="OptionArgumentsMaximumExceeded">
<source>Option '{0}' expects at most {1} arguments but {2} were provided.</source>
<target state="new">Option '{0}' expects at most {1} arguments but {2} were provided.</target>
<note />
</trans-unit>
<trans-unit id="OptionExpectsOneArgument">
<source>Option '{0}' expects a single argument but {1} were provided.</source>
<target state="new">Option '{0}' expects a single argument but {1} were provided.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/System.CommandLine/Properties/xlf/Resources.ru.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@
<target state="translated">Недопустимый символ в пути: '{0}'.</target>
<note />
</trans-unit>
<trans-unit id="OptionArgumentsMaximumExceeded">
<source>Option '{0}' expects at most {1} arguments but {2} were provided.</source>
<target state="new">Option '{0}' expects at most {1} arguments but {2} were provided.</target>
<note />
</trans-unit>
<trans-unit id="OptionExpectsOneArgument">
<source>Option '{0}' expects a single argument but {1} were provided.</source>
<target state="new">Option '{0}' expects a single argument but {1} were provided.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/System.CommandLine/Properties/xlf/Resources.tr.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@
<target state="translated">Yolda '{0}' karakterine izin verilmiyor.</target>
<note />
</trans-unit>
<trans-unit id="OptionArgumentsMaximumExceeded">
<source>Option '{0}' expects at most {1} arguments but {2} were provided.</source>
<target state="new">Option '{0}' expects at most {1} arguments but {2} were provided.</target>
<note />
</trans-unit>
<trans-unit id="OptionExpectsOneArgument">
<source>Option '{0}' expects a single argument but {1} were provided.</source>
<target state="new">Option '{0}' expects a single argument but {1} were provided.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/System.CommandLine/Properties/xlf/Resources.zh-Hans.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@
<target state="translated">路径中不允许的字符: '{0}'.</target>
<note />
</trans-unit>
<trans-unit id="OptionArgumentsMaximumExceeded">
<source>Option '{0}' expects at most {1} arguments but {2} were provided.</source>
<target state="new">Option '{0}' expects at most {1} arguments but {2} were provided.</target>
<note />
</trans-unit>
<trans-unit id="OptionExpectsOneArgument">
<source>Option '{0}' expects a single argument but {1} were provided.</source>
<target state="new">Option '{0}' expects a single argument but {1} were provided.</target>
Expand Down
5 changes: 5 additions & 0 deletions src/System.CommandLine/Properties/xlf/Resources.zh-Hant.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@
<target state="translated">路徑中不允許的字元: '{0}'.</target>
<note />
</trans-unit>
<trans-unit id="OptionArgumentsMaximumExceeded">
<source>Option '{0}' expects at most {1} arguments but {2} were provided.</source>
<target state="new">Option '{0}' expects at most {1} arguments but {2} were provided.</target>
<note />
</trans-unit>
<trans-unit id="OptionExpectsOneArgument">
<source>Option '{0}' expects a single argument but {1} were provided.</source>
<target state="new">Option '{0}' expects a single argument but {1} were provided.</target>
Expand Down