Introduce Directive Symbol type #2063
Conversation
|
|
||
| [Fact] | ||
| public void Directives_can_be_disabled() | ||
| public void When_directives_are_not_enabled_they_are_treated_as_regular_tokens() |
There was a problem hiding this comment.
I would expect them to be present as tokens now in any case, though maybe only on the root command result.
src/System.CommandLine/Directive.cs
Outdated
|
|
||
| /// <summary> | ||
| /// Method executed when given Directive is being parsed. | ||
| /// Useful for Directives that want to perform an action without setting the Handler for ParseResult. |
There was a problem hiding this comment.
This makes me wonder how this would interact with a handler.
- Is it always called?
- Is it called during parsing (as the name implies)? We should avoid encouraging side effects during parsing.
Also (maybe related), how do we want to surface the difference between directives are "handlers" and completely take over the behavior of the app (e.g. [parse]) versus directives that have some orthogonal behavior (e.g. [env]).
There was a problem hiding this comment.
If the command line has { "[Directive:one]", "[Directive]", "[Directive:three]" }, then does the implementation call OnParsed thrice:
- with DirectiveResult { Token: "[Directive:one]", Tokens: { }, Values: { "one" } }
- with DirectiveResult { Token: "[Directive:one]", Tokens: { "[Directive]" }, Values: { "one" } }
- with DirectiveResult { Token: "[Directive:one]", Tokens: { "[Directive]", "[Directive:three]" }, Values: { "one", "three" } }
So, if OnParsed wants the directive value from the token that caused it to be called, then it should ignore DirectiveResult.Values and look at the last token in DirectiveResult.Tokens, or if that is empty, then DirectiveResult.Token. If this is how it works, it's really weird.
There was a problem hiding this comment.
I feel that I'd prefer it if command-line invocation processed directives in the order they are specified by the user, rather than based on any predefined precedence. This includes interleaved directives { "[a:1]", "[b:2]", "[a:3]" }. The ParseResult.FindResultFor(Directive) method and the DirectiveResult type don't seem a good fit for that kind of processing.
There was a problem hiding this comment.
I've removed the OnParsed, but it required an ugly hack. The directive handler is now calling the parsed command handler in a direct way:
context.ParseResult.CommandResult.Command.Handler?.Invoke(context);It's just a workaround, we need a better way to chain the handlers and also a possibility to not do that.
…er (a dirty solution)
# Conflicts: # src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt # src/System.CommandLine.Benchmarks/CommandLine/Perf_Parser_ParseResult.cs # src/System.CommandLine.Tests/DirectiveTests.cs # src/System.CommandLine/Builder/CommandLineBuilder.cs # src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs # src/System.CommandLine/CommandLineConfiguration.cs # src/System.CommandLine/Invocation/ParseDirectiveResult.cs # src/System.CommandLine/Invocation/SuggestDirectiveResult.cs # src/System.CommandLine/ParseResult.cs # src/System.CommandLine/Parsing/ParseOperation.cs
…and_options_within_a_command_does_not_matter was calling Directive.GetDefaultName and throwing
introduce public APIs for setting the handler to make it possible to use reference to "this" in Directive handlers
…o pass next handler and invoke it or not
…bility to pass next handler and invoke it or not" This reverts commit fc987d4.
| } | ||
|
|
||
| // we need a cleaner, more flexible and intuitive way of continuing the execution | ||
| context.ParseResult.CommandResult.Command.Handler?.Invoke(context); |
There was a problem hiding this comment.
@jonsequitur I've reverted the ability to handle continuations and hardcoded it for now as we have agreed
adamsitnik
force-pushed
the
directives
branch
from
580635d
to
f37d8df
Compare
|
|
||
| CultureInfo.CurrentUICulture = new CultureInfo(cultureName); | ||
|
|
||
| await next?.InvokeAsync(ctx, ct); |
There was a problem hiding this comment.
If next is null, then await next?.InvokeAsync(ctx, ct) means await (Task)null, which throws NullReferenceException.
There was a problem hiding this comment.
C# doesn't yet have null-conditional await dotnet/csharplang#35.
| } | ||
| finally | ||
| { | ||
| CultureInfo.CurrentUICulture = cultureBefore; |
There was a problem hiding this comment.
On .NET Core 3 or greater, this doesn't even need to restore CultureInfo.CurrentUICulture, because the runtime keeps the value in an AsyncLocal<CultureInfo> and it cannot propagate to the caller of the async lambda in any case.
On .NET Framework, if the application targets something lower than .NET Framework 4.6, then CultureInfo.CurrentUICulture is thread-local by default. But in that environment, this can restore the culture to the wrong thread anyway.
# Conflicts: # src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt # src/System.CommandLine.Tests/DirectiveTests.cs # src/System.CommandLine/Parsing/StringExtensions.cs # src/System.CommandLine/Parsing/SymbolResultExtensions.cs
|
The API merged today is not final, it will be heavily influenced by #2071 |
fixes #2056