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

adamsitnik · 2023-02-03T11:36:35Z

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

report parsing errors as soon as unmatched token is found

make TypoCorrection a static class as all it needs is inside InvocationContext

* make internal properties fields to avoid method JITting * reduce the number of fields * provide defautl exit codes in explicit way

adamsitnik · 2023-02-03T11:39:11Z

src/System.CommandLine.Tests/ParseDirectiveTests.cs

        }

+        [Fact]
+        public async Task When_parse_directive_is_used_the_help_is_not_displayed()


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

adamsitnik · 2023-02-03T11:39:41Z

src/System.CommandLine/Builder/CommandLineBuilder.cs

        }

-        internal HelpOption? HelpOption { get; set; }
+        internal HelpOption? HelpOption;


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

adamsitnik · 2023-02-03T11:40:30Z

src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs

        public static CommandLineBuilder UseParseDirective(
            this CommandLineBuilder builder,
-            int? errorExitCode = null)
+            int errorExitCode = 1)


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

command-line-api/src/System.CommandLine/Invocation/ParseDirectiveResult.cs

Line 17 in 7030070

: errorExitCode ?? 1;

adamsitnik · 2023-02-03T11:40:51Z

src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs

        public static CommandLineBuilder UseParseErrorReporting(
            this CommandLineBuilder builder,
-            int? errorExitCode = null)
+            int errorExitCode = 1)


command-line-api/src/System.CommandLine/Invocation/ParseErrorResult.cs

Line 23 in 7030070

context.ExitCode = errorExitCode ?? 1;

adamsitnik · 2023-02-03T11:41:30Z

src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs

            int maxLevenshteinDistance = 3)
        {
-            builder.AddMiddleware(async (context, next) =>
+            if (maxLevenshteinDistance <= 0)


we should throw immediately rather than when doing first correction:

command-line-api/src/System.CommandLine/Invocation/TypoCorrection.cs

Lines 14 to 19 in 7030070

public TypoCorrection(int maxLevenshteinDistance)

{

if (maxLevenshteinDistance <= 0)

{

throw new ArgumentOutOfRangeException(nameof(maxLevenshteinDistance));

}

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

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?)

adamsitnik · 2023-02-03T11:43:47Z

src/System.CommandLine/Invocation/TypoCorrection.cs

-        private readonly int _maxLevenshteinDistance;
-
-        public TypoCorrection(int maxLevenshteinDistance)
+        internal static void ProvideSuggestions(InvocationContext context)


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

adamsitnik · 2023-02-03T11:44:32Z

src/System.CommandLine/ParseResult.cs

-            {
-                _unmatchedTokens = unmatchedTokens;
-
-                if (parser.Configuration.RootCommand.TreatUnmatchedTokensAsErrors)


we now do it as soon as the unmatched token is found. Bonus: better ordering of errors

adamsitnik · 2023-02-03T11:45:18Z

src/System.CommandLine/Parsing/ParseOperation.cs

            while (More(out TokenType currentTokenType) && currentTokenType == TokenType.Directive)
            {
-                ParseDirective(); // kept in separate method to avoid JIT
+                if (_configuration.EnableDirectives)


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

jonsequitur · 2023-02-03T16:04:29Z

src/System.CommandLine/Builder/CommandLineBuilder.cs

    public class CommandLineBuilder 
    {
+        /// <inheritdoc cref="CommandLineConfiguration.EnablePosixBundling"/>
+        internal bool EnablePosixBundling = true;


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

@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 · 2023-02-03T17:26:24Z

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 · 2023-02-03T17:28:56Z

@jonsequitur thank you very much for the review!

adamsitnik added 10 commits February 3, 2023 08:06

move HelpOption out of middleware

98311ec

move environment variable directive out of middleware

4023a31

move parse directive out of middleware

24d2d33

add a tests for combinding [perse] with --help and --version

bea86a2

refactor

e54d965

move suggest directive out of middleware

19be15b

move parse error reporting out of middleware

4053a9e

report parsing errors as soon as unmatched token is found

move typo corrections out of middleware

846233d

make TypoCorrection a static class as all it needs is inside InvocationContext

move exception handling out of middleware

cbadeca

refactoring:

aadf6c9

* make internal properties fields to avoid method JITting * reduce the number of fields * provide defautl exit codes in explicit way

adamsitnik requested a review from jonsequitur February 3, 2023 11:36

adamsitnik commented Feb 3, 2023

View reviewed changes

jonsequitur reviewed Feb 3, 2023

View reviewed changes

jonsequitur approved these changes Feb 3, 2023

View reviewed changes

adamsitnik merged commit 1bc03fe into dotnet:main Feb 3, 2023

adamsitnik deleted the lessMiddleware branch February 3, 2023 17:29

adamsitnik mentioned this pull request Feb 14, 2023

More design questions #2046

Closed

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

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

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

Uh oh!

Conversation

adamsitnik commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik commented Feb 3, 2023

Uh oh!

adamsitnik commented Feb 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adamsitnik commented Feb 3, 2023 •

edited

Loading