-
Notifications
You must be signed in to change notification settings - Fork 385
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
replace IConsole with two TextWriters in Configuration #2106
Conversation
…<string> as it allows for less
it allows us for creating the host and everything else after parsing has finished UseCommandHandler must specify the handler for instance of a Command, not per type as there can be multiple commands of the same type
# Conflicts: # src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt # src/System.CommandLine.Tests/Invocation/InvocationPipelineTests.cs # src/System.CommandLine/Builder/CommandLineBuilder.cs # src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs # src/System.CommandLine/CommandLineConfiguration.cs
# Conflicts: # src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt # src/System.CommandLine.Generator.Tests/GeneratedCommandHandlerTests.cs # src/System.CommandLine.Hosting/HostingAction.cs # src/System.CommandLine.NamingConventionBinder/ServiceProvider.cs # src/System.CommandLine.Tests/CommandExtensionsTests.cs # src/System.CommandLine.Tests/Help/HelpBuilderTests.Customization.cs # src/System.CommandLine.Tests/Invocation/InvocationPipelineTests.cs # src/System.CommandLine.Tests/UseExceptionHandlerTests.cs # src/System.CommandLine.Tests/UseHelpTests.cs # src/System.CommandLine/Help/HelpOption.cs # src/System.CommandLine/Help/HelpOptionAction.cs # src/System.CommandLine/Invocation/InvocationContext.cs
@@ -85,11 +80,13 @@ System.CommandLine | |||
public System.Collections.Generic.IReadOnlyList<Directive> Directives { get; } | |||
public System.Boolean EnablePosixBundling { get; } | |||
public System.Boolean EnableTokenReplacement { get; } | |||
public System.IO.TextWriter Error { get; set; } | |||
public System.IO.TextWriter Out { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The names Error
and Out
might need a little more context unless we're also renaming CommandLineConfiguration
, e.g. ConsoleOut
and ConsoleError
.
@adamsitnik @jonsequitur how does standard input fit into this? |
We never arrived at a satisfactory design for these interfaces. We decided it's safest to leave it out of scope for now. |
That makes sense. My main rule of API design: when in doubt, leave it out. This seems somewhat awkward:
|
It is a little awkward. We're working on how to better separate parser configuration from invocation-related concerns. Suggestions are welcome. |
The whole idea behind this PR is to move
IConsole
to Rendering and extend the Config with two mutableTextWriter
properties (Out
andError
). Both properties are lazy initialized (perf), with default values ofConsole.Out
andConsole.Error
. To test the produced output, they can be preconfigured withStringWriter
instances. To disable the ouput, they can be set toTextWriter.Null
.contributes to #1905, it's a first step towards making configuration mutable