Skip to content
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

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

Merged
merged 10 commits into from
Feb 3, 2023

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Feb 3, 2023

Method Args Mean before Mean after Allocated before Allocated after
DefaultsSync String[0] 29.035 us 26.464 us 10,264 B 7,960 B
DefaultsAsync String[0] 29.850 us 26.742 us 10,264 B 7,960 B
MinimalSync String[0] 1.316 us 1.321 us 3,264 B 3,336 B
MinimalAsync String[0] 1.448 us 1.445 us 3,264 B 3,336 B
DefaultsSync String[4] 31.213 us 28.696 us 11,760 B 9,456 B
DefaultsAsync String[4] 31.244 us 29.251 us 11,760 B 9,456 B
MinimalSync String[4] 2.415 us 2.365 us 4,760 B 4,832 B
MinimalAsync String[4] 2.497 us 2.535 us 4,760 B 4,832 B

@@ -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

@@ -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

@@ -475,19 +436,9 @@ public static CommandLineBuilder UseHelp(this CommandLineBuilder builder, int? m
/// <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

@@ -500,19 +451,9 @@ public static CommandLineBuilder UseHelp(this CommandLineBuilder builder, int? m
/// <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;

@@ -562,17 +482,12 @@ public static CommandLineBuilder UseHelp(this CommandLineBuilder builder, int? m
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?)

private readonly int _maxLevenshteinDistance;

public TypoCorrection(int maxLevenshteinDistance)
internal static void ProvideSuggestions(InvocationContext context)
Copy link
Member Author

Choose a reason for hiding this comment

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

InvocationContext provides all we need, the class and method can be static now (fewer allocs)

{
_unmatchedTokens = unmatchedTokens;

if (parser.Configuration.RootCommand.TreatUnmatchedTokensAsErrors)
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 now do it as soon as the unmatched token is found. Bonus: better ordering of errors

@@ -277,7 +285,12 @@ private void ParseDirectives()
{
while (More(out TokenType currentTokenType) && currentTokenType == TokenType.Directive)
{
ParseDirective(); // kept in separate method to avoid JIT
if (_configuration.EnableDirectives)
Copy link
Member Author

Choose a reason for hiding this comment

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

don't even try to parse the directives if the user has not asked for them (perf)

@@ -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

@adamsitnik
Copy link
Member Author

Updated benchmark numbers:

Method Args Mean before Mean after Allocated before Allocated after
DefaultsSync String[0] 29.035 us 26.464 us 10,264 B 7,960 B
DefaultsAsync String[0] 29.850 us 26.742 us 10,264 B 7,960 B
MinimalSync String[0] 1.316 us 1.321 us 3,264 B 3,336 B
MinimalAsync String[0] 1.448 us 1.445 us 3,264 B 3,336 B
DefaultsSync String[4] 31.213 us 28.696 us 11,760 B 9,456 B
DefaultsAsync String[4] 31.244 us 29.251 us 11,760 B 9,456 B
MinimalSync String[4] 2.415 us 2.365 us 4,760 B 4,832 B
MinimalAsync String[4] 2.497 us 2.535 us 4,760 B 4,832 B

@adamsitnik
Copy link
Member Author

@jonsequitur thank you very much for the review!

@adamsitnik adamsitnik merged commit 1bc03fe into dotnet:main Feb 3, 2023
@adamsitnik adamsitnik deleted the lessMiddleware branch February 3, 2023 17:29
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.

2 participants