Skip to content

all default value factories should be generic #1968

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 1 commit into from
Nov 17, 2022
Merged

all default value factories should be generic #1968

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
18 changes: 15 additions & 3 deletions src/Common/ArgumentBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,25 @@ public static Argument CreateArgument(Type valueType, string name = "value")
var argumentType = typeof(Argument<>).MakeGenericType(valueType);

#if NET6_0_OR_GREATER
var ctor = (ConstructorInfo)argumentType.GetMemberWithSameMetadataDefinitionAs(_ctor);
var ctor = (ConstructorInfo)argumentType.GetMemberWithSameMetadataDefinitionAs(_ctor);
#else
var ctor = argumentType.GetConstructor(new[] { typeof(string), typeof(string) });
#endif

var option = (Argument)ctor.Invoke(new object[] { name, null });
return (Argument)ctor.Invoke(new object[] { name, null });
}

internal static Argument CreateArgument(ParameterInfo argsParam)
{
if (!argsParam.HasDefaultValue)
{
return CreateArgument(argsParam.ParameterType, argsParam.Name);
}

var argumentType = typeof(Argument<>).MakeGenericType(argsParam.ParameterType);

var ctor = argumentType.GetConstructor(new[] { typeof(string), argsParam.ParameterType, typeof(string) });

return option;
return (Argument)ctor.Invoke(new object[] { argsParam.Name, argsParam.DefaultValue, null });
}
}
30 changes: 28 additions & 2 deletions src/Common/OptionBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ static OptionBuilder()
_ctor = typeof(Option<string>).GetConstructor(new[] { typeof(string), typeof(string) });
}

public static Option CreateOption(string name, Type valueType)
public static Option CreateOption(string name, Type valueType, string description = null)
{
var optionType = typeof(Option<>).MakeGenericType(valueType);

Expand All @@ -24,8 +24,34 @@ public static Option CreateOption(string name, Type valueType)
var ctor = optionType.GetConstructor(new[] { typeof(string), typeof(string) });
#endif

var option = (Option)ctor.Invoke(new object[] { name, null });
var option = (Option)ctor.Invoke(new object[] { name, description });

return option;
}

public static Option CreateOption(string name, Type valueType, string description, Func<object> defaultValueFactory)
{
if (defaultValueFactory == null)
{
return CreateOption(name, valueType, description);
}

var optionType = typeof(Bridge<>).MakeGenericType(valueType);

var ctor = optionType.GetConstructor(new[] { typeof(string), typeof(Func<object>), typeof(string) });

var option = (Option)ctor.Invoke(new object[] { name, defaultValueFactory, description });

return option;
}

private class Bridge<T> : Option<T>
{
public Bridge(string name, Func<object> defaultValueFactory, string description)
: base(name,
() => (T)defaultValueFactory(), // this type exists only for an easy Func<object> => Func<T> transformation
description)
{
}
}
}
12 changes: 7 additions & 5 deletions ...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 @@ -9,17 +9,16 @@ System.CommandLine
public System.Object GetDefaultValue()
public ParseResult Parse(System.String commandLine)
public ParseResult Parse(System.String[] args)
public System.Void SetDefaultValue(System.Object value)
public System.Void SetDefaultValueFactory(System.Func<System.Object> defaultValueFactory)
public System.Void SetDefaultValueFactory(System.Func<System.CommandLine.Parsing.ArgumentResult,System.Object> defaultValueFactory)
public System.String ToString()
public class Argument<T> : Argument, IValueDescriptor<T>, System.CommandLine.Binding.IValueDescriptor
.ctor()
.ctor(System.String name, System.String description = null)
.ctor(System.String name, Func<T> defaultValueFactory, System.String description = null)
.ctor(System.String name, T defaultValue, System.String description = null)
.ctor(Func<T> defaultValueFactory)
.ctor(System.String name, Func<System.CommandLine.Parsing.ArgumentResult,T> parse, System.Boolean isDefault = False, System.String description = null)
.ctor(Func<System.CommandLine.Parsing.ArgumentResult,T> parse, System.Boolean isDefault = False)
public System.Boolean HasDefaultValue { get; }
public System.Type ValueType { get; }
public Argument<T> AcceptLegalFileNamesOnly()
public Argument<T> AcceptLegalFilePathsOnly()
Expand All @@ -28,6 +27,9 @@ System.CommandLine
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)
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)
public struct ArgumentArity : System.ValueType, System.IEquatable<ArgumentArity>
public static ArgumentArity ExactlyOne { get; }
public static ArgumentArity OneOrMore { get; }
Expand Down Expand Up @@ -197,8 +199,6 @@ System.CommandLine
public System.Boolean HasAliasIgnoringPrefix(System.String alias)
public ParseResult Parse(System.String commandLine)
public ParseResult Parse(System.String[] args)
public System.Void SetDefaultValue(System.Object value)
public System.Void SetDefaultValueFactory(System.Func<System.Object> defaultValueFactory)
public class Option<T> : Option, IValueDescriptor<T>, System.CommandLine.Binding.IValueDescriptor
.ctor(System.String name, System.String description = null)
.ctor(System.String[] aliases, System.String description = null)
Expand All @@ -213,6 +213,8 @@ System.CommandLine
public Option<T> AddCompletions(System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.String>> completionsDelegate)
public Option<T> AddCompletions(System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>> completionsDelegate)
public Option<T> AddValidator(System.Action<System.CommandLine.Parsing.OptionResult> validate)
public System.Void SetDefaultValue(T value)
public System.Void SetDefaultValueFactory(Func<T> defaultValueFactory)
public static class OptionValidation
public static Option<System.IO.FileInfo> AcceptExistingOnly(this Option<System.IO.FileInfo> option)
public static Option<System.IO.DirectoryInfo> AcceptExistingOnly(this Option<System.IO.DirectoryInfo> option)
Expand Down
39 changes: 6 additions & 33 deletions src/System.CommandLine.DragonFruit/CommandLine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,21 +165,7 @@ public static void ConfigureFromMethod(

if (method.GetParameters().FirstOrDefault(p => _argumentParameterNames.Contains(p.Name)) is { } argsParam)
{
var argument = ArgumentBuilder.CreateArgument(argsParam.ParameterType, argsParam.Name);

if (argsParam.HasDefaultValue)
{
if (argsParam.DefaultValue is not null)
{
argument.SetDefaultValue(argsParam.DefaultValue);
}
else
{
argument.SetDefaultValueFactory(() => null);
}
}

command.AddArgument(argument);
command.AddArgument(ArgumentBuilder.CreateArgument(argsParam));
}

command.Handler = CommandHandler.Create(method, target);
Expand Down Expand Up @@ -285,24 +271,11 @@ public static IEnumerable<Option> BuildOptions(this MethodInfo method)
}

public static Option BuildOption(this ParameterDescriptor parameter)
{
Func<object> getDefaultValue = null;
if (parameter.HasDefaultValue)
{
getDefaultValue = parameter.GetDefaultValue;
}

var option = OptionBuilder.CreateOption(parameter.BuildAlias(), parameter.ValueType);

option.Description = parameter.ValueName;

if (getDefaultValue is not null)
{
option.SetDefaultValueFactory(getDefaultValue);
}

return option;
}
=> OptionBuilder.CreateOption(
parameter.BuildAlias(),
parameter.ValueType,
parameter.ValueName,
parameter.HasDefaultValue ? parameter.GetDefaultValue : null);

private static string GetDefaultXmlDocsFileLocation(Assembly assembly)
{
Expand Down
47 changes: 2 additions & 45 deletions src/System.CommandLine/Argument.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ namespace System.CommandLine
/// </summary>
public abstract class Argument : Symbol, IValueDescriptor
{
private Func<ArgumentResult, object?>? _defaultValueFactory;
private ArgumentArity _arity;
private TryConvertArgument? _convertArguments;
private List<Func<CompletionContext, IEnumerable<CompletionItem>>>? _completions = null;
Expand Down Expand Up @@ -115,54 +114,12 @@ private protected override string DefaultName
return GetDefaultValue(new ArgumentResult(this, null));
}

internal object? GetDefaultValue(ArgumentResult argumentResult)
{
if (_defaultValueFactory is null)
{
throw new InvalidOperationException($"Argument \"{Name}\" does not have a default value");
}

return _defaultValueFactory.Invoke(argumentResult);
}

/// <summary>
/// Sets the default value for the argument.
/// </summary>
/// <param name="value">The default value for the argument.</param>
public void SetDefaultValue(object? value)
{
SetDefaultValueFactory(() => value);
}

/// <summary>
/// Sets a delegate to invoke when the default value for the argument is required.
/// </summary>
/// <param name="defaultValueFactory">The delegate to invoke to return the default value.</param>
/// <exception cref="ArgumentNullException">Thrown when <paramref name="defaultValueFactory"/> is null.</exception>
public void SetDefaultValueFactory(Func<object?> defaultValueFactory)
{
if (defaultValueFactory is null)
{
throw new ArgumentNullException(nameof(defaultValueFactory));
}

SetDefaultValueFactory(_ => defaultValueFactory());
}

/// <summary>
/// Sets a delegate to invoke when the default value for the argument is required.
/// </summary>
/// <param name="defaultValueFactory">The delegate to invoke to return the default value.</param>
/// <remarks>In this overload, the <see cref="ArgumentResult"/> is provided to the delegate.</remarks>
public void SetDefaultValueFactory(Func<ArgumentResult, object?> defaultValueFactory)
{
_defaultValueFactory = defaultValueFactory ?? throw new ArgumentNullException(nameof(defaultValueFactory));
}
internal abstract object? GetDefaultValue(ArgumentResult argumentResult);

/// <summary>
/// Specifies if a default value is defined for the argument.
/// </summary>
public bool HasDefaultValue => _defaultValueFactory is not null;
public abstract bool HasDefaultValue { get; }

internal virtual bool HasCustomParser => false;

Expand Down
76 changes: 64 additions & 12 deletions src/System.CommandLine/Argument{T}.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ namespace System.CommandLine
/// <inheritdoc cref="Argument" />
public class Argument<T> : Argument, IValueDescriptor<T>
{
private Func<ArgumentResult, T>? _defaultValueFactory;
private readonly bool _hasCustomParser;

/// <summary>
Expand Down Expand Up @@ -40,26 +41,30 @@ public Argument(
Func<T> defaultValueFactory,
string? description = null) : this(name, description)
{
if (defaultValueFactory is null)
{
throw new ArgumentNullException(nameof(defaultValueFactory));
}

SetDefaultValueFactory(() => defaultValueFactory());
Copy link

Choose a reason for hiding this comment

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

Why did you delete this null check? ArgumentNullException is still documented but the method no longer throws it. A null argument will instead be saved in the closure and cause NullReferenceException if the factory is called later.

Copy link

Choose a reason for hiding this comment

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

The same comment applies to the public Argument(Func<T> defaultValueFactory) constructor.

}

/// <summary>
/// Initializes a new instance of the Argument class.
/// </summary>
/// <param name="name">The name of the argument.</param>
/// <param name="defaultValue">The default value.</param>
/// <param name="description">The description of the argument, shown in help.</param>
public Argument(
string name,
T defaultValue,
string? description = null) : this(name, description)
{
SetDefaultValue(defaultValue);
}

/// <summary>
/// Initializes a new instance of the Argument class.
/// </summary>
/// <param name="defaultValueFactory">The delegate to invoke to return the default value.</param>
/// <exception cref="ArgumentNullException">Thrown when <paramref name="defaultValueFactory"/> is null.</exception>
public Argument(Func<T> defaultValueFactory) : this()
{
if (defaultValueFactory is null)
{
throw new ArgumentNullException(nameof(defaultValueFactory));
}

SetDefaultValueFactory(() => defaultValueFactory());
}

Expand All @@ -73,7 +78,7 @@ public Argument(Func<T> defaultValueFactory) : this()
/// <exception cref="ArgumentNullException">Thrown when <paramref name="parse"/> is null.</exception>
public Argument(
string? name,
Func<ArgumentResult, T> parse,
Func<ArgumentResult, T> parse,
bool isDefault = false,
string? description = null) : this(name, description)
{
Expand All @@ -84,7 +89,7 @@ public Argument(

if (isDefault)
{
SetDefaultValueFactory(argumentResult => parse(argumentResult));
SetDefaultValueFactory(parse);
}

ConvertArguments = (ArgumentResult argumentResult, out object? value) =>
Expand Down Expand Up @@ -120,6 +125,53 @@ public Argument(Func<ArgumentResult, T> parse, bool isDefault = false) : this(nu
/// <inheritdoc />
public override Type ValueType => typeof(T);

/// <inheritdoc />
public override bool HasDefaultValue => _defaultValueFactory is not null;

/// <summary>
/// Sets the default value for the argument.
/// </summary>
/// <param name="value">The default value for the argument.</param>
public void SetDefaultValue(T value)
{
SetDefaultValueFactory(() => value);
}

/// <summary>
/// Sets a delegate to invoke when the default value for the argument is required.
/// </summary>
/// <param name="defaultValueFactory">The delegate to invoke to return the default value.</param>
/// <exception cref="ArgumentNullException">Thrown when <paramref name="defaultValueFactory"/> is null.</exception>
public void SetDefaultValueFactory(Func<T> defaultValueFactory)
{
if (defaultValueFactory is null)
{
throw new ArgumentNullException(nameof(defaultValueFactory));
}

SetDefaultValueFactory(_ => defaultValueFactory());
}

/// <summary>
/// Sets a delegate to invoke when the default value for the argument is required.
/// </summary>
/// <param name="defaultValueFactory">The delegate to invoke to return the default value.</param>
/// <remarks>In this overload, the <see cref="ArgumentResult"/> is provided to the delegate.</remarks>
public void SetDefaultValueFactory(Func<ArgumentResult, T> defaultValueFactory)
{
_defaultValueFactory = defaultValueFactory ?? throw new ArgumentNullException(nameof(defaultValueFactory));
}

internal override object? GetDefaultValue(ArgumentResult argumentResult)
{
if (_defaultValueFactory is null)
{
throw new InvalidOperationException($"Argument \"{Name}\" does not have a default value");
}

return _defaultValueFactory.Invoke(argumentResult);
}

/// <summary>
/// Adds completions for the argument.
/// </summary>
Expand Down
15 changes: 0 additions & 15 deletions src/System.CommandLine/Option.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,21 +121,6 @@ public bool HasAliasIgnoringPrefix(string alias)
return false;
}

/// <summary>
/// Sets the default value for the option.
/// </summary>
/// <param name="value">The default value for the option.</param>
public void SetDefaultValue(object? value) =>
Argument.SetDefaultValue(value);

/// <summary>
/// Sets a delegate to invoke when the default value for the option is required.
/// </summary>
/// <param name="defaultValueFactory">The delegate to invoke to return the default value.</param>
/// <exception cref="ArgumentNullException">Thrown when <paramref name="defaultValueFactory"/> is null.</exception>
public void SetDefaultValueFactory(Func<object?> defaultValueFactory) =>
Argument.SetDefaultValueFactory(defaultValueFactory);

/// <summary>
/// Gets a value that indicates whether multiple argument tokens are allowed for each option identifier token.
/// </summary>
Expand Down
Loading