Skip to content

Move most of the built in middlewares out of middleware #2043

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 10 commits into from
Feb 3, 2023
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ System.CommandLine
public static CommandLineBuilder UseHelp(this CommandLineBuilder builder, System.Action<System.CommandLine.Help.HelpContext> customize, System.Nullable<System.Int32> maxWidth = null)
public static TBuilder UseHelpBuilder<TBuilder>(this TBuilder builder, System.Func<System.CommandLine.Binding.BindingContext,System.CommandLine.Help.HelpBuilder> getHelpBuilder)
public static CommandLineBuilder UseLocalizationResources(this CommandLineBuilder builder, LocalizationResources validationMessages)
public static CommandLineBuilder UseParseDirective(this CommandLineBuilder builder, System.Nullable<System.Int32> errorExitCode = null)
public static CommandLineBuilder UseParseErrorReporting(this CommandLineBuilder builder, System.Nullable<System.Int32> errorExitCode = null)
public static CommandLineBuilder UseParseDirective(this CommandLineBuilder builder, System.Int32 errorExitCode = 1)
public static CommandLineBuilder UseParseErrorReporting(this CommandLineBuilder builder, System.Int32 errorExitCode = 1)
public static CommandLineBuilder UseSuggestDirective(this CommandLineBuilder builder)
public static CommandLineBuilder UseTokenReplacer(this CommandLineBuilder builder, System.CommandLine.Parsing.TryReplaceToken replaceToken)
public static CommandLineBuilder UseTypoCorrections(this CommandLineBuilder builder, System.Int32 maxLevenshteinDistance = 3)
Expand Down
2 changes: 1 addition & 1 deletion src/System.CommandLine.Hosting.Tests/HostingHandlerTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public static async Task Parameter_is_available_in_property()
}

[Fact]
public static async Task Can_have_diferent_handlers_based_on_command()
public static async Task Can_have_different_handlers_based_on_command()
{
var root = new RootCommand();

Expand Down
50 changes: 50 additions & 0 deletions src/System.CommandLine.Tests/ParseDirectiveTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,56 @@ public async Task Parse_directive_writes_parse_diagram()
.Be($"![ {RootCommand.ExecutableName} [ subcommand [ -c <34> ] ] ] ???--> --nonexistent wat" + Environment.NewLine);
}

[Fact]
public async Task When_parse_directive_is_used_the_help_is_not_displayed()
Copy link
Member Author

Choose a reason for hiding this comment

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

we were missing test coverage scenario for cases when users combine [parse] and --help or --version

{
var rootCommand = new RootCommand();

var parser = new CommandLineBuilder(rootCommand)
.UseParseDirective()
.UseVersionOption()
.UseHelp()
.Build();

var result = parser.Parse("[parse] --help");

output.WriteLine(result.Diagram());

var console = new TestConsole();

await result.InvokeAsync(console);

console.Out
.ToString()
.Should()
.Be($"[ {RootCommand.ExecutableName} [ --help ] ]" + Environment.NewLine);
}

[Fact]
public async Task When_parse_directive_is_used_the_version_is_not_displayed()
{
var rootCommand = new RootCommand();

var parser = new CommandLineBuilder(rootCommand)
.UseParseDirective()
.UseVersionOption()
.UseHelp()
.Build();

var result = parser.Parse("[parse] --version");

output.WriteLine(result.Diagram());

var console = new TestConsole();

await result.InvokeAsync(console);

console.Out
.ToString()
.Should()
.Be($"[ {RootCommand.ExecutableName} [ --version ] ]" + Environment.NewLine);
}

[Fact]
public async Task When_there_are_no_errors_then_parse_directive_sets_exit_code_0()
{
Expand Down
54 changes: 37 additions & 17 deletions src/System.CommandLine/Builder/CommandLineBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,33 @@ namespace System.CommandLine
/// </summary>
public class CommandLineBuilder
{
/// <inheritdoc cref="CommandLineConfiguration.EnablePosixBundling"/>
internal bool EnablePosixBundling = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Because it's a field, the naming convention would typically be _enablePosixBundling.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jonsequitur for now I would prefer to stay with the current name, as I expect that next week we are going to introduce mutable CommandLineConfiguration and just turn these fields into public properties


/// <inheritdoc cref="CommandLineConfiguration.EnableTokenReplacement"/>
internal bool EnableTokenReplacement = true;

/// <inheritdoc cref="CommandLineConfiguration.ParseErrorReportingExitCode"/>
internal int? ParseErrorReportingExitCode;

/// <inheritdoc cref="CommandLineConfiguration.EnableDirectives"/>
internal bool EnableDirectives = true;

/// <inheritdoc cref="CommandLineConfiguration.EnableEnvironmentVariableDirective"/>
internal bool EnableEnvironmentVariableDirective;

/// <inheritdoc cref="CommandLineConfiguration.ParseDirectiveExitCode"/>
internal int? ParseDirectiveExitCode;

/// <inheritdoc cref="CommandLineConfiguration.EnableSuggestDirective"/>
internal bool EnableSuggestDirective;

/// <inheritdoc cref="CommandLineConfiguration.MaxLevenshteinDistance"/>
internal int MaxLevenshteinDistance;

/// <inheritdoc cref="CommandLineConfiguration.ExceptionHandler"/>
internal Action<Exception, InvocationContext>? ExceptionHandler;

// for every generic type with type argument being struct JIT needs to compile a dedicated version
// (because each struct is of a different size)
// that is why we don't use List<ValueTuple> for middleware
Expand All @@ -33,19 +60,6 @@ public CommandLineBuilder(Command? rootCommand = null)
/// </summary>
public Command Command { get; }

/// <summary>
/// Determines whether the parser recognizes command line directives.
/// </summary>
/// <seealso cref="DirectiveCollection"/>
internal bool EnableDirectives { get; set; } = true;

/// <summary>
/// Determines whether the parser recognize and expands POSIX-style bundled options.
/// </summary>
internal bool EnablePosixBundling { get; set; } = true;

internal bool EnableTokenReplacement { get; set; } = true;

internal void CustomizeHelpLayout(Action<HelpContext> customize) =>
_customizeHelpBuilder = customize;

Expand All @@ -68,19 +82,19 @@ HelpBuilder CreateHelpBuilder(BindingContext bindingContext)
}
}

internal HelpOption? HelpOption { get; set; }
internal HelpOption? HelpOption;
Copy link
Member Author

Choose a reason for hiding this comment

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

fields: no need to JIT a property. A micro optimization for internal properites only


internal VersionOption? VersionOption { get; set; }
internal VersionOption? VersionOption;

internal int? MaxHelpWidth { get; set; }
internal int? MaxHelpWidth;

internal LocalizationResources LocalizationResources
{
get => _localizationResources ??= LocalizationResources.Instance;
set => _localizationResources = value;
}

internal TryReplaceToken? TokenReplacer { get; set; }
internal TryReplaceToken? TokenReplacer;

/// <summary>
/// Creates a parser based on the configuration of the command line builder.
Expand All @@ -92,6 +106,12 @@ public Parser Build() =>
enablePosixBundling: EnablePosixBundling,
enableDirectives: EnableDirectives,
enableTokenReplacement: EnableTokenReplacement,
enableEnvironmentVariableDirective: EnableEnvironmentVariableDirective,
parseDirectiveExitCode: ParseDirectiveExitCode,
enableSuggestDirective: EnableSuggestDirective,
parseErrorReportingExitCode: ParseErrorReportingExitCode,
maxLevenshteinDistance: MaxLevenshteinDistance,
exceptionHandler: ExceptionHandler,
resources: LocalizationResources,
middlewarePipeline: _middlewareList is null
? Array.Empty<InvocationMiddleware>()
Expand Down
134 changes: 15 additions & 119 deletions src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
using System.CommandLine.IO;
using System.CommandLine.Parsing;
using System.IO;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using static System.Environment;
Expand Down Expand Up @@ -230,27 +229,7 @@ await feature.EnsureRegistered(async () =>
public static CommandLineBuilder UseEnvironmentVariableDirective(
this CommandLineBuilder builder)
{
builder.AddMiddleware((context, next) =>
{
if (context.ParseResult.Directives.TryGetValue("env", out var keyValuePairs))
{
for (var i = 0; i < keyValuePairs.Count; i++)
{
var envDirective = keyValuePairs[i];
var components = envDirective.Split(new[] { '=' }, count: 2);
var variable = components.Length > 0 ? components[0].Trim() : string.Empty;
if (string.IsNullOrEmpty(variable) || components.Length < 2)
{
continue;
}

var value = components[1].Trim();
SetEnvironmentVariable(variable, value);
}
}

return next(context);
}, MiddlewareOrderInternal.EnvironmentVariableDirective);
builder.EnableEnvironmentVariableDirective = true;

return builder;
}
Expand Down Expand Up @@ -302,17 +281,7 @@ public static CommandLineBuilder UseExceptionHandler(
Action<Exception, InvocationContext>? onException = null,
int? errorExitCode = null)
{
builder.AddMiddleware(async (context, next) =>
{
try
{
await next(context);
}
catch (Exception exception)
{
(onException ?? Default)(exception, context);
}
}, MiddlewareOrderInternal.ExceptionHandler);
builder.ExceptionHandler = onException ?? Default;

return builder;

Expand Down Expand Up @@ -397,14 +366,6 @@ internal static CommandLineBuilder UseHelp(
builder.HelpOption = helpOption;
builder.Command.AddGlobalOption(helpOption);
builder.MaxHelpWidth = maxWidth;

builder.AddMiddleware(async (context, next) =>
{
if (!ShowHelp(context, builder.HelpOption))
{
await next(context);
}
}, MiddlewareOrderInternal.HelpOption);
}
return builder;
}
Expand Down Expand Up @@ -475,19 +436,9 @@ public static CommandLineBuilder AddMiddleware(
/// <returns>The same instance of <see cref="CommandLineBuilder"/>.</returns>
public static CommandLineBuilder UseParseDirective(
this CommandLineBuilder builder,
int? errorExitCode = null)
int errorExitCode = 1)
Copy link
Member Author

Choose a reason for hiding this comment

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

IMO it's better to make the default value explicit. It comes from

{
builder.AddMiddleware(async (context, next) =>
{
if (context.ParseResult.Directives.ContainsKey("parse"))
{
context.InvocationResult = ctx => ParseDirectiveResult.Apply(ctx, errorExitCode);
}
else
{
await next(context);
}
}, MiddlewareOrderInternal.ParseDirective);
builder.ParseDirectiveExitCode = errorExitCode;

return builder;
}
Expand All @@ -500,19 +451,9 @@ public static CommandLineBuilder UseParseDirective(
/// <returns>The same instance of <see cref="CommandLineBuilder"/>.</returns>
public static CommandLineBuilder UseParseErrorReporting(
this CommandLineBuilder builder,
int? errorExitCode = null)
int errorExitCode = 1)
Copy link
Member Author

Choose a reason for hiding this comment

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

context.ExitCode = errorExitCode ?? 1;

{
builder.AddMiddleware(async (context, next) =>
{
if (context.ParseResult.Errors.Count > 0)
{
context.InvocationResult = ctx => ParseErrorResult.Apply(ctx, errorExitCode);
}
else
{
await next(context);
}
}, MiddlewareOrderInternal.ParseErrorReporting);
builder.ParseErrorReportingExitCode = errorExitCode;

return builder;
}
Expand All @@ -526,28 +467,7 @@ public static CommandLineBuilder UseParseErrorReporting(
public static CommandLineBuilder UseSuggestDirective(
this CommandLineBuilder builder)
{
builder.AddMiddleware(async (context, next) =>
{
if (context.ParseResult.Directives.TryGetValue("suggest", out var values))
{
int position;

if (values.FirstOrDefault() is { } positionString)
{
position = int.Parse(positionString);
}
else
{
position = context.ParseResult.CommandLineText?.Length ?? 0;
}

context.InvocationResult = ctx => SuggestDirectiveResult.Apply(ctx, position);
}
else
{
await next(context);
}
}, MiddlewareOrderInternal.SuggestDirective);
builder.EnableSuggestDirective = true;

return builder;
}
Expand All @@ -562,17 +482,12 @@ public static CommandLineBuilder UseTypoCorrections(
this CommandLineBuilder builder,
int maxLevenshteinDistance = 3)
{
builder.AddMiddleware(async (context, next) =>
if (maxLevenshteinDistance <= 0)
Copy link
Member Author

Choose a reason for hiding this comment

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

we should throw immediately rather than when doing first correction:

public TypoCorrection(int maxLevenshteinDistance)
{
if (maxLevenshteinDistance <= 0)
{
throw new ArgumentOutOfRangeException(nameof(maxLevenshteinDistance));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me that this even needs to be configurable. I'd recommend hard-coding the distance.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's add it to a list of braking changes related to configuration and include in one PR with config re-design (somewhen next week?)

{
if (context.ParseResult.CommandResult.Command.TreatUnmatchedTokensAsErrors &&
context.ParseResult.UnmatchedTokens.Count > 0)
{
var typoCorrection = new TypoCorrection(maxLevenshteinDistance);
throw new ArgumentOutOfRangeException(nameof(maxLevenshteinDistance));
}

typoCorrection.ProvideSuggestions(context.ParseResult, context.Console);
}
await next(context);
}, MiddlewareOrderInternal.TypoCorrection);
builder.MaxLevenshteinDistance = maxLevenshteinDistance;

return builder;
}
Expand Down Expand Up @@ -619,10 +534,8 @@ public static CommandLineBuilder UseVersionOption(
return builder;
}

var versionOption = new VersionOption(builder);

builder.VersionOption = versionOption;
builder.Command.Options.Add(versionOption);
builder.VersionOption = new (builder);
builder.Command.Options.Add(builder.VersionOption);

return builder;
}
Expand All @@ -634,32 +547,15 @@ public static CommandLineBuilder UseVersionOption(
this CommandLineBuilder builder,
params string[] aliases)
{
var command = builder.Command;

if (builder.VersionOption is not null)
{
return builder;
}

var versionOption = new VersionOption(aliases, builder);

builder.VersionOption = versionOption;
command.Options.Add(versionOption);
builder.VersionOption = new (aliases, builder);
builder.Command.Options.Add(builder.VersionOption);

return builder;
}

private static bool ShowHelp(
InvocationContext context,
Option helpOption)
{
if (context.ParseResult.FindResultFor(helpOption) is { })
{
context.InvocationResult = HelpResult.Apply;
return true;
}

return false;
}
}
}
Loading