Skip to content

Separate parse from invocation configurations #2606

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Separate parse from invocation configurations #2606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
bf132ef
restore System.CommandLine.v3.ncrunchsolution
jonsequitur Jun 30, 2025
58b691e
split out configuration types for Parse vs Invoke
jonsequitur Jul 1, 2025
932fea1
remove Output and Error from CommandLineConfiguration
jonsequitur Jul 1, 2025
8167444
remove CommandLineConfiguration.RootCommand
jonsequitur Jul 1, 2025
92b00cd
update API baseline
jonsequitur Jul 1, 2025
8685f9d
rename CommandLineConfiguration to ParserConfiguration
jonsequitur Jul 1, 2025
1737e33
rename CommandLineInvocationConfiguration to InvocationConfiguration
jonsequitur Jul 1, 2025
5d2c622
rename CommandLineConfigurationException to ParserConfigurationException
jonsequitur Jul 1, 2025
937491a
update API baseline
jonsequitur Jul 1, 2025
997cebe
fix a couple of warnings
jonsequitur Jul 1, 2025
2c6d6a3
Update src/System.CommandLine/ParseResult.cs
jonsequitur Jul 2, 2025
9bfecf6
PR comments, API baseline update
jonsequitur Jul 2, 2025
1f37d7d
fix test app code
jonsequitur Jul 2, 2025
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
12 changes: 12 additions & 0 deletions System.CommandLine.v3.ncrunchsolution
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<SolutionConfiguration>
<Settings>
<AllowParallelTestExecution>True</AllowParallelTestExecution>
<CustomBuildProperties>
<Value>TargetFrameworks = net8.0</Value>
<Value>TargetFramework = net8.0</Value>
</CustomBuildProperties>
<EnableRDI>False</EnableRDI>
<RdiConfigured>True</RdiConfigured>
<SolutionConfigured>True</SolutionConfigured>
</Settings>
</SolutionConfiguration>
44 changes: 21 additions & 23 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 @@ -49,31 +49,13 @@
public System.Void Add(Option option)
public System.Void Add(Command command)
public System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem> GetCompletions(System.CommandLine.Completions.CompletionContext context)
public ParseResult Parse(System.Collections.Generic.IReadOnlyList<System.String> args, CommandLineConfiguration configuration = null)
public ParseResult Parse(System.String commandLine, CommandLineConfiguration configuration = null)
public ParseResult Parse(System.Collections.Generic.IReadOnlyList<System.String> args, ParserConfiguration configuration = null)
public ParseResult Parse(System.String commandLine, ParserConfiguration configuration = null)
public System.Void SetAction(System.Action<ParseResult> action)
public System.Void SetAction(System.Func<ParseResult,System.Int32> action)
public System.Void SetAction(System.Func<ParseResult,System.Threading.CancellationToken,System.Threading.Tasks.Task> action)
public System.Void SetAction(System.Func<ParseResult,System.Threading.Tasks.Task> action)
public System.Void SetAction(System.Func<ParseResult,System.Threading.CancellationToken,System.Threading.Tasks.Task<System.Int32>> action)
public class CommandLineConfiguration
.ctor(Command rootCommand)
public System.Boolean EnableDefaultExceptionHandler { get; set; }
public System.Boolean EnablePosixBundling { get; set; }
public System.IO.TextWriter Error { get; set; }
public System.IO.TextWriter Output { get; set; }
public System.Nullable<System.TimeSpan> ProcessTerminationTimeout { get; set; }
public System.CommandLine.Parsing.TryReplaceToken ResponseFileTokenReplacer { get; set; }
public Command RootCommand { get; }
public System.Int32 Invoke(System.String commandLine)
public System.Int32 Invoke(System.String[] args)
public System.Threading.Tasks.Task<System.Int32> InvokeAsync(System.String commandLine, System.Threading.CancellationToken cancellationToken = null)
public System.Threading.Tasks.Task<System.Int32> InvokeAsync(System.String[] args, System.Threading.CancellationToken cancellationToken = null)
public ParseResult Parse(System.Collections.Generic.IReadOnlyList<System.String> args)
public ParseResult Parse(System.String commandLine)
public System.Void ThrowIfInvalid()
public class CommandLineConfigurationException : System.Exception, System.Runtime.Serialization.ISerializable
.ctor(System.String message)
public static class CompletionSourceExtensions
public static System.Void Add(this System.Collections.Generic.List<System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>>> completionSources, System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.String>> completionsDelegate)
public static System.Void Add(this System.Collections.Generic.List<System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>>> completionSources, System.String[] completions)
Expand All @@ -88,6 +70,12 @@
public class EnvironmentVariablesDirective : Directive
.ctor()
public System.CommandLine.Invocation.CommandLineAction Action { get; set; }
public class InvocationConfiguration
.ctor()
public System.Boolean EnableDefaultExceptionHandler { get; set; }
public System.IO.TextWriter Error { get; set; }
public System.IO.TextWriter Output { get; set; }
public System.Nullable<System.TimeSpan> ProcessTerminationTimeout { get; set; }
public abstract class Option : Symbol
public System.CommandLine.Invocation.CommandLineAction Action { get; set; }
public System.Collections.Generic.ICollection<System.String> Aliases { get; }
Expand Down Expand Up @@ -115,11 +103,19 @@
public static Option<System.IO.DirectoryInfo> AcceptExistingOnly(this Option<System.IO.DirectoryInfo> option)
public static Option<System.IO.FileSystemInfo> AcceptExistingOnly(this Option<System.IO.FileSystemInfo> option)
public static Option<T> AcceptExistingOnly<T>(this Option<T> option)
public class ParserConfiguration
.ctor()
public System.Boolean EnablePosixBundling { get; set; }
public System.CommandLine.Parsing.TryReplaceToken ResponseFileTokenReplacer { get; set; }
public System.Void ThrowIfInvalid(Command command)
public class ParserConfigurationException : System.Exception, System.Runtime.Serialization.ISerializable
Copy link
Member

Choose a reason for hiding this comment

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

I almost forgot about #1909. Should we simply remove this custom exception type and throw something defined by the BCL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same thought. I'd love suggestions on what would be an appropriate exception type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KalleOlaviNiemitalo has a very good breakdown here: #1909 (comment)

Copy link
Member

@adamsitnik adamsitnik Jul 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

InvalidOperationException would be my best bet for now.

Or... even just removing ThrowIfInvalid? We don't use it anywhere outside of tests and it can be implemented with the public APIs we provide. It's also a bit confusing that it's part of the Config class, while what it does it checks the symbol hierarchy (and RootCommand is no longer part of the config)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this suggestion. This method has always been awkward.

We used to do this validation at runtime. Then we decided it was too performance-intensive so we said it should be used only in tests. My worry has been that people will assume the parser will catch these kinds of mistakes automatically, and it's not intuitive to implement some of the checks. (And I'm pretty sure we missed some cases.) Of course, ASP.NET left the analogous issue (routing validation) as an exercise for the user, so probably System.CommandLine should do the same.

.ctor(System.String message)
public class ParseResult
public System.CommandLine.Invocation.CommandLineAction Action { get; }
public System.CommandLine.Parsing.CommandResult CommandResult { get; }
public CommandLineConfiguration Configuration { get; }
public ParserConfiguration Configuration { get; }
public System.Collections.Generic.IReadOnlyList<System.CommandLine.Parsing.ParseError> Errors { get; }
public InvocationConfiguration InvocationConfiguration { get; }
public System.CommandLine.Parsing.CommandResult RootCommandResult { get; }
public System.Collections.Generic.IReadOnlyList<System.CommandLine.Parsing.Token> Tokens { get; }
public System.Collections.Generic.IReadOnlyList<System.String> UnmatchedTokens { get; }
Expand All @@ -138,7 +134,9 @@
public T GetValue<T>(Option<T> option)
public T GetValue<T>(System.String name)
public System.Int32 Invoke()
public System.Int32 Invoke(InvocationConfiguration configuration)
public System.Threading.Tasks.Task<System.Int32> InvokeAsync(System.Threading.CancellationToken cancellationToken = null)
public System.Threading.Tasks.Task<System.Int32> InvokeAsync(InvocationConfiguration configuration, System.Threading.CancellationToken cancellationToken = null)
Copy link
Member

Choose a reason for hiding this comment

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

For Parse methods, the configuration argument is optional (null by default, mapped into default config by the implementation). Perhaps Invoke should follow the same patter for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or should we change Parse to this overload approach versus coalescing the null parameter at runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If consistency is important here, another reason we might consider consolidating by splitting out Parse into two overloads (Parse() and Parse(ParserConfiguration)) is that that would not be a code-breaking change. Invoke has an InvokeAsync variant and this already has an optional CancellationToken argument, so adding an overload avoids breaking existing callers, since the InvocationConfiguration parameter comes before the CancellationToken parameter. (Of course there are a lot of breaking changes here so maybe this doesn't matter.)

Copy link
Member

Choose a reason for hiding this comment

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

Or should we change Parse to this overload approach versus coalescing the null parameter at runtime?

I strongly believe that we should not do that. IIRC this shape was approved in the API Design, it would be another breaking change and also another public API.

Last, but not least my goal with exposing Parse(CommandLineConfiguration? config = null) as the only overload was to make it easier for the end users to discover the fact that CommandLineConfiguration exists (in the past, it was quite hard to find out how to customize parsing: you would need to know about the existance of builder type etc).

Copy link
Member

Choose a reason for hiding this comment

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

Invoke has an InvokeAsync variant and this already has an optional CancellationToken argument, so adding an overload avoids breaking existing callers, since the InvocationConfiguration parameter comes before the CancellationToken parameter. (Of course there are a lot of breaking changes here so maybe this doesn't matter.)

That is very good point. However, from my observation it seems that users hardly ever provide CancellationToken to InvokeAsync as they usually call it from Main where they don't have any token.

public System.String ToString()
public class RootCommand : Command, System.Collections.IEnumerable
public static System.String ExecutableName { get; }
Expand Down Expand Up @@ -212,8 +210,8 @@ System.CommandLine.Parsing
public System.Void OnlyTake(System.Int32 numberOfTokens)
public System.String ToString()
public static class CommandLineParser
public static System.CommandLine.ParseResult Parse(System.CommandLine.Command command, System.Collections.Generic.IReadOnlyList<System.String> args, System.CommandLine.CommandLineConfiguration configuration = null)
public static System.CommandLine.ParseResult Parse(System.CommandLine.Command command, System.String commandLine, System.CommandLine.CommandLineConfiguration configuration = null)
public static System.CommandLine.ParseResult Parse(System.CommandLine.Command command, System.Collections.Generic.IReadOnlyList<System.String> args, System.CommandLine.ParserConfiguration configuration = null)
public static System.CommandLine.ParseResult Parse(System.CommandLine.Command command, System.String commandLine, System.CommandLine.ParserConfiguration configuration = null)
public static System.Collections.Generic.IEnumerable<System.String> SplitCommandLine(System.String commandLine)
public class CommandResult : SymbolResult
public System.Collections.Generic.IEnumerable<SymbolResult> Children { get; }
Expand Down
4 changes: 1 addition & 3 deletions src/System.CommandLine.Suggest.Tests/EndToEndTestApp/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,7 @@ static async Task Main(string[] args)
return Task.CompletedTask;
});

CommandLineConfiguration commandLine = new (rootCommand);

await commandLine.InvokeAsync(args);
await rootCommand.Parse(args).InvokeAsync();
}
}
}
35 changes: 17 additions & 18 deletions src/System.CommandLine.Suggest.Tests/SuggestionShellScriptHandlerTest.cs
View file
Open in desktop
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.IO;
using System.Threading;
using System.Threading.Tasks;
using FluentAssertions;
using Xunit;
Expand All @@ -10,32 +11,34 @@ namespace System.CommandLine.Suggest.Tests
{
public class SuggestionShellScriptHandlerTest
{
private readonly CommandLineConfiguration _configuration;
private readonly RootCommand _rootCommand;
private readonly InvocationConfiguration _configuration;

public SuggestionShellScriptHandlerTest()
{
_configuration = new SuggestionDispatcher(new TestSuggestionRegistration()).Configuration;
_rootCommand = new SuggestionDispatcher(new TestSuggestionRegistration()).RootCommand;
_configuration = new()
{
Output = new StringWriter(),
Error = new StringWriter()
};
}

[Fact]
public async Task When_shell_type_is_not_supported_it_throws()
{
_configuration.Error = new StringWriter();

await _configuration.InvokeAsync("script 123");
await _rootCommand.Parse("script 123").InvokeAsync(_configuration, CancellationToken.None);

_configuration.Error
.ToString()
.Should()
.Contain("Shell '123' is not supported.");
.ToString()
.Should()
.Contain("Shell '123' is not supported.");
}

[Fact]
public async Task It_should_print_bash_shell_script()
{
_configuration.Output = new StringWriter();

await _configuration.InvokeAsync("script bash");
await _rootCommand.Parse("script bash").InvokeAsync(_configuration, CancellationToken.None);

_configuration.Output.ToString().Should().Contain("_dotnet_bash_complete()");
_configuration.Output.ToString().Should().NotContain("\r\n");
Expand All @@ -44,9 +47,7 @@ public async Task It_should_print_bash_shell_script()
[Fact]
public async Task It_should_print_powershell_shell_script()
{
_configuration.Output = new StringWriter();

await _configuration.InvokeAsync("script powershell");
await _rootCommand.Parse("script powershell").InvokeAsync(_configuration, CancellationToken.None);

_configuration.Output.ToString().Should().Contain("Register-ArgumentCompleter");
_configuration.Output.ToString().Should().Contain("\r\n");
Expand All @@ -55,12 +56,10 @@ public async Task It_should_print_powershell_shell_script()
[Fact]
public async Task It_should_print_zsh_shell_script()
{
_configuration.Output = new StringWriter();
await _rootCommand.Parse("script zsh").InvokeAsync(_configuration, CancellationToken.None);

await _configuration.InvokeAsync("script zsh");

_configuration.Output.ToString().Should().Contain("_dotnet_zsh_complete()");
_configuration.Output.ToString().Should().NotContain("\r\n");
}
}
}
}
21 changes: 11 additions & 10 deletions src/System.CommandLine.Suggest/SuggestionDispatcher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using System.CommandLine.Completions;

namespace System.CommandLine.Suggest
{
Expand All @@ -29,7 +28,7 @@ public SuggestionDispatcher(ISuggestionRegistration suggestionRegistration, ISug
};
CompleteScriptCommand.SetAction(context =>
{
SuggestionShellScriptHandler.Handle(context.Configuration.Output, context.GetValue(shellTypeArgument));
SuggestionShellScriptHandler.Handle(context.InvocationConfiguration.Output, context.GetValue(shellTypeArgument));
});

ListCommand = new Command("list")
Expand All @@ -38,7 +37,7 @@ public SuggestionDispatcher(ISuggestionRegistration suggestionRegistration, ISug
};
ListCommand.SetAction((ctx, cancellationToken) =>
{
ctx.Configuration.Output.WriteLine(ShellPrefixesToMatch(_suggestionRegistration));
ctx.InvocationConfiguration.Output.WriteLine(ShellPrefixesToMatch(_suggestionRegistration));
return Task.CompletedTask;
});

Expand All @@ -59,25 +58,27 @@ public SuggestionDispatcher(ISuggestionRegistration suggestionRegistration, ISug

RegisterCommand.SetAction((context, cancellationToken) =>
{
Register(context.GetValue(commandPathOption), context.Configuration.Output);
Register(context.GetValue(commandPathOption), context.InvocationConfiguration.Output);
return Task.CompletedTask;
});

var root = new RootCommand
RootCommand = new RootCommand
{
ListCommand,
GetCommand,
RegisterCommand,
CompleteScriptCommand,
};
root.TreatUnmatchedTokensAsErrors = false;
Configuration = new CommandLineConfiguration(root);
RootCommand.TreatUnmatchedTokensAsErrors = false;
Configuration = new InvocationConfiguration();
}

private Command CompleteScriptCommand { get; }

private Command GetCommand { get; }

public RootCommand RootCommand { get; }

private Option<FileInfo> ExecutableOption { get; } = GetExecutableOption();

private static Option<FileInfo> GetExecutableOption()
Expand All @@ -98,11 +99,11 @@ private static Option<FileInfo> GetExecutableOption()

private Command RegisterCommand { get; }

public CommandLineConfiguration Configuration { get; }
public InvocationConfiguration Configuration { get; }

public TimeSpan Timeout { get; set; } = TimeSpan.FromMilliseconds(5000);

public Task<int> InvokeAsync(string[] args) => Configuration.InvokeAsync(args);
public Task<int> InvokeAsync(string[] args) => RootCommand.Parse(args).InvokeAsync(Configuration);

private void Register(
string commandPath,
Expand Down Expand Up @@ -168,7 +169,7 @@ private Task<int> Get(ParseResult parseResult, CancellationToken cancellationTok
Program.LogDebug($"dotnet-suggest returning: \"{completions.Replace("\r", "\\r").Replace("\n", "\\n")}\"");
#endif

parseResult.Configuration.Output.Write(completions);
parseResult.InvocationConfiguration.Output.Write(completions);

return Task.FromResult(0);
}
Expand Down
2 changes: 1 addition & 1 deletion src/System.CommandLine.Tests/Binding/TestModels.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public class ClassWithMethodHavingParameter<T>

public ClassWithMethodHavingParameter(ParseResult parseResult)
{
_output = parseResult.Configuration.Output;
_output = parseResult.InvocationConfiguration.Output;
}

public int Handle(T value)
Expand Down
Loading