Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Next Next commit
expose Argument.Validators, remove AddValidator helper method
  • Loading branch information
adamsitnik committed Dec 9, 2022
commit df9f188c614591afe64dac7af5b04ea3c3157b7b
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ System.CommandLine
public System.Collections.Generic.ICollection<System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>>> Completions { get; }
public System.Boolean HasDefaultValue { get; }
public System.String HelpName { get; set; }
public System.Collections.Generic.List<System.Action<System.CommandLine.Parsing.ArgumentResult>> Validators { get; }
public System.Type ValueType { get; }
public System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem> GetCompletions(System.CommandLine.Completions.CompletionContext context)
Copy link
Member

Choose a reason for hiding this comment

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

The difference between GetCompletions() and Completions is that the former is the result of evaluating the Funcs in the latter, right? Shouldn't we disambiguate? Maybe we can rename the List property to CompletionSources.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jozkee that is a very good point!

@KathleenDollard what is your opinion on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

Can you search the code to ensure we do not already have something called "CompletionSources" as I vaguely remember that. We should update the List name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome, I've applied the rename and the PR is ready for review again. Thank you both for your feedback!

public System.Object GetDefaultValue()
Expand All @@ -26,7 +27,6 @@ System.CommandLine
public Argument<T> AddCompletions(System.String[] completions)
public Argument<T> AddCompletions(System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.String>> completionsDelegate)
public Argument<T> AddCompletions(System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>> completionsDelegate)
Copy link
Member

Choose a reason for hiding this comment

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

?

Suggested change
public Argument<T> AddCompletions(System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.String>> completionsDelegate)
public Argument<T> AddCompletions(System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>> completionsDelegate)

Copy link
Member Author

Choose a reason for hiding this comment

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

@jozkee great catch, #solved

public Argument<T> AddValidator(System.Action<System.CommandLine.Parsing.ArgumentResult> validate)
public System.Void SetDefaultValue(T value)
public System.Void SetDefaultValueFactory(Func<T> defaultValueFactory)
public System.Void SetDefaultValueFactory(Func<System.CommandLine.Parsing.ArgumentResult,T> defaultValueFactory)
Expand Down
1 change: 0 additions & 1 deletion src/System.CommandLine.Tests/ArgumentTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,6 @@ public void Argument_of_T_fluent_APIs_return_Argument_of_T()
.AddCompletions("test")
.AddCompletions(ctx => Array.Empty<string>())
.AddCompletions(ctx => Array.Empty<CompletionItem>())
.AddValidator(_ => { })
.AcceptLegalFileNamesOnly()
.AcceptLegalFilePathsOnly();

Expand Down
12 changes: 6 additions & 6 deletions src/System.CommandLine.Tests/ParsingValidationTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ public void A_custom_validator_can_be_added_to_an_argument()
{
var argument = new Argument<int>("x");

argument.AddValidator(r =>
argument.Validators.Add(r =>
{
var value = r.GetValueOrDefault<int>();

Expand Down Expand Up @@ -365,7 +365,7 @@ public void All_custom_validators_are_called(string commandLine)
});

var argument = new Argument<string>("the-arg");
argument.AddValidator(_ =>
argument.Validators.Add(_ =>
{
argumentValidatorWasCalled = true;
});
Expand Down Expand Up @@ -460,7 +460,7 @@ public void Custom_validator_error_messages_are_not_repeated()
{
var errorMessage = "that's not right...";
var argument = new Argument<string>();
argument.AddValidator(r => r.ErrorMessage = errorMessage);
argument.Validators.Add(r => r.ErrorMessage = errorMessage);

var cmd = new Command("get")
{
Expand All @@ -481,7 +481,7 @@ public void The_parsed_value_of_an_argument_is_available_within_a_validator()
{
var argument = new Argument<int>();
var errorMessage = "The value of option '-x' must be between 1 and 100.";
argument.AddValidator(result =>
argument.Validators.Add(result =>
{
var value = result.GetValue(argument);

Expand Down Expand Up @@ -1162,8 +1162,8 @@ 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.AddValidator(result => result.ErrorMessage = "Wrong");
argument.AddValidator(_ => { });
argument.Validators.Add(result => result.ErrorMessage = "Wrong");
argument.Validators.Add(_ => { });

var command = new RootCommand
{
Expand Down
6 changes: 5 additions & 1 deletion src/System.CommandLine/Argument.cs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,11 @@ private protected override string DefaultName
}
}

internal List<Action<ArgumentResult>> Validators => _validators ??= new ();
/// <summary>
/// Provides a list of argument validators. Validators can be used
/// to provide custom errors based on user input.
/// </summary>
public List<Action<ArgumentResult>> Validators => _validators ??= new ();

/// <summary>
/// Gets the default value for the argument.
Expand Down
12 changes: 6 additions & 6 deletions src/System.CommandLine/ArgumentValidation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public static class ArgumentValidation
/// <returns>The configured argument.</returns>
public static Argument<FileInfo> AcceptExistingOnly(this Argument<FileInfo> argument)
{
argument.AddValidator(Validate.FileExists);
argument.Validators.Add(Validate.FileExists);
return argument;
}

Expand All @@ -29,7 +29,7 @@ public static Argument<FileInfo> AcceptExistingOnly(this Argument<FileInfo> argu
/// <returns>The configured argument.</returns>
public static Argument<DirectoryInfo> AcceptExistingOnly(this Argument<DirectoryInfo> argument)
{
argument.AddValidator(Validate.DirectoryExists);
argument.Validators.Add(Validate.DirectoryExists);
return argument;
}

Expand All @@ -40,7 +40,7 @@ public static Argument<DirectoryInfo> AcceptExistingOnly(this Argument<Directory
/// <returns>The configured argument.</returns>
public static Argument<FileSystemInfo> AcceptExistingOnly(this Argument<FileSystemInfo> argument)
{
argument.AddValidator(Validate.FileOrDirectoryExists);
argument.Validators.Add(Validate.FileOrDirectoryExists);
return argument;
}

Expand All @@ -54,15 +54,15 @@ public static Argument<T> AcceptExistingOnly<T>(this Argument<T> argument)
{
if (typeof(IEnumerable<FileInfo>).IsAssignableFrom(typeof(T)))
{
argument.AddValidator(Validate.FileExists);
argument.Validators.Add(Validate.FileExists);
}
else if (typeof(IEnumerable<DirectoryInfo>).IsAssignableFrom(typeof(T)))
{
argument.AddValidator(Validate.DirectoryExists);
argument.Validators.Add(Validate.DirectoryExists);
}
else
{
argument.AddValidator(Validate.FileOrDirectoryExists);
argument.Validators.Add(Validate.FileOrDirectoryExists);
}

return argument;
Expand Down
15 changes: 2 additions & 13 deletions src/System.CommandLine/Argument{T}.cs
Original file line number Diff line number Diff line change
Expand Up @@ -205,17 +205,6 @@ public Argument<T> AddCompletions(Func<CompletionContext, IEnumerable<Completion
return this;
}

/// <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 Argument<T> AddValidator(Action<ArgumentResult> validate)
{
Validators.Add(validate);
return this;
}

/// <summary>
/// Configures the argument to accept only the specified values, and to suggest them as command line completions.
/// </summary>
Expand All @@ -239,7 +228,7 @@ public Argument<T> AcceptLegalFilePathsOnly()
{
var invalidPathChars = Path.GetInvalidPathChars();

AddValidator(result =>
Validators.Add(result =>
{
for (var i = 0; i < result.Tokens.Count; i++)
{
Expand Down Expand Up @@ -268,7 +257,7 @@ public Argument<T> AcceptLegalFileNamesOnly()
{
var invalidFileNameChars = Path.GetInvalidFileNameChars();

AddValidator(result =>
Validators.Add(result =>
{
for (var i = 0; i < result.Tokens.Count; i++)
{
Expand Down