Skip to content

Remove Argument.AllowedValues, use Validator to implement the logic #1959

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 6 commits into from
Dec 15, 2022

Conversation

adamsitnik
Copy link
Member

follow up to #1891 (comment)

@@ -83,7 +83,7 @@ public void OnlyTake(int numberOfTokens)

if (!string.IsNullOrWhiteSpace(ErrorMessage))
{
return new ParseError(ErrorMessage!, this);
return new ParseError(ErrorMessage!, Parent is OptionResult option ? option : this);
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is not obvious. Before this change, the SymbolResult of ParseError for an Argument with unmatched tokens owned by an Option was the OptionResult, not ArgumentResult:

argumentResult.Parent?.UnrecognizedArgumentError(argument) ??

but this was true only "non-custom errors" (user defined, coming from validators):

argumentResult.CustomError(argument);

Without this change, one test was failing:

  Failed System.CommandLine.Tests.ParseDiagramTests.Parse_result_diagram_displays_unmatched_tokens [283 ms]
  Error Message:
   Expected string to be
"[ command ![ -x <ar> ] ]", but
"[ command [ -x !<ar> ] ]" differs near "[ -" (index 10).

Since we hide the Option.Argument from the user, I think it's reasonable to use the OptionResult in such cases for all validations.

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Nov 11, 2022

AcceptOnlyFromAmong is case-sensitive, unlike the Enum.Parse call in ArgumentConverter

return Success(argument, Enum.Parse(type, value, true));
, so it is not a good method for restricting which enum values can be used. This PR kind of improves the situation by making AcceptOnlyFromAmong use the same features that are in the public API, so an app developer can more easily imitate this when implementing a case-insensitive variant.

@jonsequitur
Copy link
Contributor

If there's no test for case-insensitive enum parsing, we should definitely add one.

@adamsitnik adamsitnik requested review from jozkee and Keboo November 15, 2022 18:39

/// <summary>
/// Adds a custom validator to the argument. Validators can be used
/// to provide custom errors based on user input.
/// </summary>
/// <param name="validate">The action to validate the parsed argument.</param>
public void AddValidator(Action<ArgumentResult> validate) => Validators.Add(validate);
public void AddValidator(Action<ArgumentResult> validate) => (_validators ??= new()).Add(validate);
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 other places that call Validator.Add, which would now add the Action to the Array.Empty<Action>.

Validators.Add(validate);

option.Argument.Validators.Add(Validate.FileExists);


/// <summary>
/// Adds a custom validator to the argument. Validators can be used
/// to provide custom errors based on user input.
/// </summary>
/// <param name="validate">The action to validate the parsed argument.</param>
public void AddValidator(Action<ArgumentResult> validate) => Validators.Add(validate);
public void AddValidator(Action<ArgumentResult> validate) => (_validators ??= new()).Add(validate);
Copy link
Member

Choose a reason for hiding this comment

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

Now that #1966 is merged, this shoud go on Argument<T>.

@adamsitnik
Copy link
Member Author

I've addressed all the feedback, I am going to merge this PR now to unblock other PRs.

AcceptOnlyFromAmong is case-sensitive, unlike the Enum.Parse call in ArgumentConverter

Please create an issue for that. We can just extend AcceptOnlyFromAmong with an StringComparison comparisonType argument and let the users decide

@adamsitnik adamsitnik merged commit d5f3c3b into dotnet:main Dec 15, 2022
@adamsitnik adamsitnik deleted the removeAllowedValues branch December 15, 2022 14:04
@KalleOlaviNiemitalo
Copy link

Please create an issue for that.

Already created #1960 a month ago.

We can just extend AcceptOnlyFromAmong with an StringComparison comparisonType argument and let the users decide

Users can easily implement AcceptOnlyFromAmong(string[], StringComparison) on their own, so I don't think the library needs to provide that. For enums though, users are more likely to get the validation wrong, because it is not evident from the method signatures that the parsing will be case-insensitive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants