Skip to content

Move version option out of middleware #1969

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
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

<PropertyGroup>
<IsPackable>true</IsPackable>
<TargetFrameworks>netstandard2.0;netstandard2.1</TargetFrameworks>
<TargetFrameworks>netstandard2.0;netstandard2.1;net7.0</TargetFrameworks>
<Description>This package provides support for using System.CommandLine with Microsoft.Extensions.Hosting.</Description>
</PropertyGroup>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -605,7 +605,7 @@ public static ICommandHandler Create<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T1
Func<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15, T16, Task<int>> action) =>
HandlerDescriptor.FromDelegate(action).GetCommandHandler();

internal static async Task<int> GetExitCodeAsync(object returnValue, InvocationContext context)
internal static async Task<int> GetExitCodeAsync(object? returnValue, InvocationContext context)
{
switch (returnValue)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public override ICommandHandler GetCommandHandler()
}
}

public override ModelDescriptor Parent => ModelDescriptor.FromType(_handlerMethodInfo.ReflectedType);
public override ModelDescriptor Parent => ModelDescriptor.FromType(_handlerMethodInfo.ReflectedType!);
Copy link
Member

Choose a reason for hiding this comment

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


private protected override IEnumerable<ParameterDescriptor> InitializeParameterDescriptors() =>
_handlerMethodInfo.GetParameters()
Expand Down
4 changes: 2 additions & 2 deletions src/System.CommandLine.NamingConventionBinder/ModelBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ private bool ShortCutTheBinding()
private (bool success, object? newInstance, bool anyNonDefaults) InstanceFromSpecificConstructor(
BindingContext bindingContext, ConstructorDescriptor constructor, IReadOnlyList<BoundValue>? boundValues, ref bool nonDefaultsUsed)
{
var values = boundValues.Select(x => x.Value).ToArray();
var values = boundValues?.Select(x => x.Value).ToArray() ?? Array.Empty<object>();
object? newInstance = null;
try
{
Expand Down Expand Up @@ -332,7 +332,7 @@ private static BoundValue DefaultForValueDescriptor(
valueSource);
}

private protected IValueDescriptor FindModelPropertyDescriptor(Type propertyType, string propertyName)
private protected IValueDescriptor? FindModelPropertyDescriptor(Type propertyType, string propertyName)
{
return ModelDescriptor.PropertyDescriptors
.FirstOrDefault(desc =>
Expand Down
19 changes: 11 additions & 8 deletions src/System.CommandLine.NamingConventionBinder/ModelBinder{T}.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@ public void BindMemberFromValue<TValue>(
IValueDescriptor valueDescriptor)
{
var (propertyType, propertyName) = property.MemberTypeAndName();
var propertyDescriptor = FindModelPropertyDescriptor(
propertyType, propertyName);
MemberBindingSources[propertyDescriptor] =
new SpecificSymbolValueSource(valueDescriptor);
var propertyDescriptor = FindModelPropertyDescriptor(propertyType, propertyName);

if (propertyDescriptor is not null)
{
MemberBindingSources[propertyDescriptor] = new SpecificSymbolValueSource(valueDescriptor);
}
}

/// <summary>
Expand All @@ -42,9 +44,10 @@ public void BindMemberFromValue<TValue>(
Func<BindingContext?, TValue> getValue)
{
var (propertyType, propertyName) = property.MemberTypeAndName();
var propertyDescriptor = FindModelPropertyDescriptor(
propertyType, propertyName);
MemberBindingSources[propertyDescriptor] =
new DelegateValueSource(c => getValue(c));
var propertyDescriptor = FindModelPropertyDescriptor(propertyType, propertyName);
if (propertyDescriptor is not null)
{
MemberBindingSources[propertyDescriptor] = new DelegateValueSource(c => getValue(c));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ internal ModelBindingCommandHandler(
_handlerMethodInfo = handlerMethodInfo ?? throw new ArgumentNullException(nameof(handlerMethodInfo));
_invocationTargetBinder = _handlerMethodInfo.IsStatic
? null
: new ModelBinder(_handlerMethodInfo.ReflectedType);
: new ModelBinder(_handlerMethodInfo.ReflectedType!);
_methodDescriptor = methodDescriptor ?? throw new ArgumentNullException(nameof(methodDescriptor));
_invocationTarget = invocationTarget;
}
Expand Down Expand Up @@ -69,11 +69,11 @@ public async Task<int> InvokeAsync(InvocationContext context)
.Select(x => x.Value)
.ToArray();

object result;
object? result;
if (_handlerDelegate is null)
{
var invocationTarget = _invocationTarget ??
bindingContext.GetService(_handlerMethodInfo!.ReflectedType);
bindingContext.GetService(_handlerMethodInfo!.ReflectedType!);
if(invocationTarget is { })
{
_invocationTargetBinder?.UpdateInstance(invocationTarget, bindingContext);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ protected ModelDescriptor(Type modelType)
public IReadOnlyList<IValueDescriptor> PropertyDescriptors =>
_propertyDescriptors ??=
ModelType.GetProperties(CommonBindingFlags)
.Where(p => p.CanWrite && p.SetMethod.IsPublic)
.Where(p => p.CanWrite && p.SetMethod?.IsPublic == true)
.Select(i => new PropertyDescriptor(i, this))
.ToList();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ internal ParameterDescriptor(
}

/// <inheritdoc />
public string ValueName => _parameterInfo.Name;
public string ValueName => _parameterInfo.Name!;

/// <summary>
/// The method descriptor that this constructor belongs to.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<PropertyGroup>
<IsPackable>true</IsPackable>
<PackageId>System.CommandLine.NamingConventionBinder</PackageId>
<TargetFramework>netstandard2.0</TargetFramework>
<TargetFrameworks>net7.0;netstandard2.0</TargetFrameworks>
<LangVersion>10</LangVersion>
<Nullable>enable</Nullable>
<Description>This package provides command handler support for System.CommandLine performs parameter and model binding by matching option and argument names to parameter and property names.</Description>
Expand Down
40 changes: 8 additions & 32 deletions src/System.CommandLine.Tests/VersionOptionTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public async Task When_the_version_option_is_specified_and_there_are_default_opt
{
var rootCommand = new RootCommand
{
new Option<bool>("-x")
new Option<bool>("-x", defaultValueFactory: () => true)
};
rootCommand.SetHandler(() => { });

Expand Down Expand Up @@ -126,21 +126,9 @@ public void Version_is_not_valid_with_other_tokens(string commandLine)
.UseVersionOption()
.Build();

var console = new TestConsole();

var result = parser.Invoke(commandLine, console);
var result = parser.Parse(commandLine);

console.Out
.ToString()
.Should()
.NotContain(version);

console.Error
.ToString()
.Should()
.Contain("--version option cannot be combined with other arguments.");

result.Should().NotBe(0);
result.Errors.Should().Contain(e => e.Message == "--version option cannot be combined with other arguments.");
}

[Fact]
Expand Down Expand Up @@ -206,7 +194,7 @@ public async Task Version_can_specify_additional_alias()
[Fact]
public void Version_is_not_valid_with_other_tokens_uses_custom_alias()
{
var childCommand = new Command("subcommand");
var childCommand = new Command("subcommand");
childCommand.SetHandler(() => { });
var rootCommand = new RootCommand
{
Expand All @@ -215,24 +203,12 @@ public void Version_is_not_valid_with_other_tokens_uses_custom_alias()
rootCommand.SetHandler(() => { });

var parser = new CommandLineBuilder(rootCommand)
.UseVersionOption("-v")
.Build();

var console = new TestConsole();

var result = parser.Invoke("-v subcommand", console);

console.Out
.ToString()
.Should()
.NotContain(version);
.UseVersionOption("-v")
.Build();

console.Error
.ToString()
.Should()
.Contain("-v option cannot be combined with other arguments.");
var result = parser.Parse("-v subcommand");

result.Should().NotBe(0);
result.Errors.Should().ContainSingle(e => e.Message == "-v option cannot be combined with other arguments.");
}
}
}
57 changes: 0 additions & 57 deletions src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
using System.CommandLine.Parsing;
using System.IO;
using System.Linq;
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;
using static System.Environment;
Expand All @@ -22,24 +21,6 @@ namespace System.CommandLine
/// </summary>
public static class CommandLineBuilderExtensions
{
private static readonly Lazy<string> _assemblyVersion =
new(() =>
{
var assembly = RootCommand.GetAssembly();

var assemblyVersionAttribute = assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>();

if (assemblyVersionAttribute is null)
{
return assembly.GetName().Version?.ToString() ?? "";
}
else
{
return assemblyVersionAttribute.InformationalVersion;
}

});

/// <summary>
/// Enables signaling and handling of process termination via a <see cref="CancellationToken"/> that can be passed to a <see cref="ICommandHandler"/> during invocation.
/// </summary>
Expand Down Expand Up @@ -643,25 +624,6 @@ public static CommandLineBuilder UseVersionOption(
builder.VersionOption = versionOption;
builder.Command.Options.Add(versionOption);

builder.AddMiddleware(async (context, next) =>
{
if (context.ParseResult.FindResultFor(versionOption) is { })
{
if (context.ParseResult.Errors.Any(e => e.SymbolResult is OptionResult optionResult && optionResult.Option is VersionOption))
{
context.InvocationResult = static ctx => ParseErrorResult.Apply(ctx, null);
}
else
{
context.Console.Out.WriteLine(_assemblyVersion.Value);
}
}
else
{
await next(context);
}
}, MiddlewareOrderInternal.VersionOption);

return builder;
}

Expand All @@ -684,25 +646,6 @@ public static CommandLineBuilder UseVersionOption(
builder.VersionOption = versionOption;
command.Options.Add(versionOption);

builder.AddMiddleware(async (context, next) =>
{
if (context.ParseResult.FindResultFor(versionOption) is { })
{
if (context.ParseResult.Errors.Any(e => e.SymbolResult is OptionResult optionResult && optionResult.Option is VersionOption))
{
context.InvocationResult = static ctx => ParseErrorResult.Apply(ctx, null);
}
else
{
context.Console.Out.WriteLine(_assemblyVersion.Value);
}
}
else
{
await next(context);
}
}, MiddlewareOrderInternal.VersionOption);

return builder;
}

Expand Down
1 change: 0 additions & 1 deletion src/System.CommandLine/Help/HelpOption.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ public HelpOption(string[] aliases, Func<LocalizationResources> getLocalizationR
: base(aliases, null, new Argument<bool> { Arity = ArgumentArity.Zero })
{
_localizationResources = getLocalizationResources;
DisallowBinding = true;
}

public HelpOption(Func<LocalizationResources> getLocalizationResources) : this(new[]
Expand Down
4 changes: 0 additions & 4 deletions src/System.CommandLine/Help/VersionOption.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,13 @@ internal class VersionOption : Option<bool>
{
_builder = builder;

DisallowBinding = true;

AddValidators();
}

public VersionOption(string[] aliases, CommandLineBuilder builder) : base(aliases)
{
_builder = builder;

DisallowBinding = true;

AddValidators();
}

Expand Down
33 changes: 13 additions & 20 deletions src/System.CommandLine/Invocation/InvocationPipeline.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Collections.Generic;
using System.CommandLine.Parsing;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
Expand All @@ -21,15 +22,15 @@ public Task<int> InvokeAsync(IConsole? console = null, CancellationToken cancell
{
var context = new InvocationContext(_parseResult, console, cancellationToken);

if (context.Parser.Configuration.Middleware.Count == 0
&& context.ParseResult.CommandResult.Command.Handler is ICommandHandler handler)
if (context.Parser.Configuration.Middleware.Count == 0 &&
_parseResult.Handler is not null)
{
return handler.InvokeAsync(context);
return _parseResult.Handler.InvokeAsync(context);
}

return FullInvocationChainAsync(context);
return InvokeHandlerWithMiddleware(context);

static async Task<int> FullInvocationChainAsync(InvocationContext context)
static async Task<int> InvokeHandlerWithMiddleware(InvocationContext context)
{
InvocationMiddleware invocationChain = BuildInvocationChain(context, true);

Expand All @@ -49,9 +50,9 @@ public int Invoke(IConsole? console = null)
return handler.Invoke(context);
}

return FullInvocationChain(context); // kept in a separate method to avoid JITting
return InvokeHandlerWithMiddleware(context); // kept in a separate method to avoid JITting

static int FullInvocationChain(InvocationContext context)
static int InvokeHandlerWithMiddleware(InvocationContext context)
{
InvocationMiddleware invocationChain = BuildInvocationChain(context, false);

Expand All @@ -68,27 +69,19 @@ private static InvocationMiddleware BuildInvocationChain(InvocationContext conte

invocations.Add(async (invocationContext, _) =>
{
if (invocationContext
.ParseResult
.CommandResult
.Command is Command command)
if (invocationContext.ParseResult.Handler is { } handler)
{
var handler = command.Handler;

if (handler is not null)
{
context.ExitCode = invokeAsync
? await handler.InvokeAsync(invocationContext)
: handler.Invoke(invocationContext);
}
context.ExitCode = invokeAsync
? await handler.InvokeAsync(invocationContext)
: handler.Invoke(invocationContext);
}
});

return invocations.Aggregate(
(first, second) =>
(ctx, next) =>
first(ctx,
c => second(c, next)));
c => second(c, next)));
}

private static int GetExitCode(InvocationContext context)
Expand Down
2 changes: 0 additions & 2 deletions src/System.CommandLine/Option.cs
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,6 @@ public ArgumentArity Arity
/// </summary>
internal bool IsGlobal { get; set; }

internal bool DisallowBinding { get; init; }

/// <summary>
/// Validators that will be called when the option is matched by the parser.
/// </summary>
Expand Down
Loading