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

Introduce Directive symbol type #2056

Closed
4 tasks
adamsitnik opened this issue Feb 17, 2023 · 9 comments · Fixed by #2063
Closed
4 tasks

Introduce Directive symbol type #2056

adamsitnik opened this issue Feb 17, 2023 · 9 comments · Fixed by #2063
Assignees

Comments

@adamsitnik
Copy link
Member

From #2046:

Currently the only way for the users to implement custom directives is enabling directives parsing in the config, using this property to get parsed output:

public IReadOnlyDictionary<string, IReadOnlyList<string>> Directives => _directives ??= new ();

and provide a custom, expensive Middleware.

We need to introduce a dedicated Directive type that derives from Symbol.

Initial requirements:

  • introducing new Directive symbol type, with a possibility to define an Action<ParseResult, string> which should be executed when directive is parsed (the list of arguments for the action is an open question, when implementing the feature it should became more obvious)
  • introducing Command.Directives property to allow users for defining directives
  • turning existing code into new directives
    private void OnDirectiveParsed(string directiveKey, string? parsedValues)
    {
    if (_configuration.EnableEnvironmentVariableDirective && directiveKey == "env")
    {
    if (parsedValues is not null)
    {
    var components = parsedValues.Split(new[] { '=' }, count: 2);
    var variable = components.Length > 0 ? components[0].Trim() : string.Empty;
    if (string.IsNullOrEmpty(variable) || components.Length < 2)
    {
    return;
    }
    var value = components[1].Trim();
    Environment.SetEnvironmentVariable(variable, value);
    }
    }
    else if (_configuration.ParseDirectiveExitCode.HasValue && directiveKey == "parse")
    {
    _isParseRequested = true;
    _handler = new AnonymousCommandHandler(ParseDirectiveResult.Apply);
    }
    else if (_configuration.EnableSuggestDirective && directiveKey == "suggest")
    {
    int position = parsedValues is not null ? int.Parse(parsedValues) : _rawInput?.Length ?? 0;
    _handler = new AnonymousCommandHandler(ctx => SuggestDirectiveResult.Apply(ctx, position));
    }
    }
  • extending tokenization and parsing with the concept of Directive
@KalleOlaviNiemitalo
Copy link

I'm surprised that you want to place it in Command.Directives rather than in the config.
If a subcommand defines directives, then do users still place those directives at the start of the command line, or after the name of the subcommand?

@adamsitnik
Copy link
Member Author

If a subcommand defines directives, then do users still place those directives at the start of the command line, or after the name of the subcommand?

They will still be supported only at the start. The problem with config is that it does not belong to symbol tree. But RootCommand does, so perhaps it should be allowed only for RootCommand?

@adamsitnik
Copy link
Member Author

#2059 was supposed to close #2058, not this usse

@jonsequitur
Copy link
Contributor

But RootCommand does, so perhaps it should be allowed only for RootCommand?

I think this is important.

We explicitly decided not to support directives anywhere on the command line after the first occurrence of a non-directive token.

@KalleOlaviNiemitalo
Copy link

We need to introduce a dedicated Directive type that derives from Symbol.

I'm not convinced that deriving Directive from Symbol is the best way to support custom directives. I'm thinking some IDictionary<string, Action<DirectiveContext>> in CommandLineConfiguration, instead. Because directives don't appear in help, cannot be placed in subcommands, and do not have names in the same namespace as subcommands and options.

If directive completion were a feature, then that could be a reason to derive from Symbol.

@adamsitnik
Copy link
Member Author

adamsitnik commented Feb 22, 2023

Because directives don't appear in help

I believe that we should change that, otherwise how users are going to find out about their existence and capabilities?

If directive completion were a feature

We should consider that too.

@jonsequitur
Copy link
Contributor

I think most end users will not care about directives (e.g. [suggest] is intended to be called from tooling, not directly by users) and many CLI authors won't want them in their help output. Maybe this is a use case for --help verbose or something similar.

@KalleOlaviNiemitalo
Copy link

Or even [help] for directive-specific help.

I feel that directives should not be specific to a command, not even to a RootCommand. Command-specific stuff should be options instead. Directives should be tied to the application model or I/O (are the command lines coming from Program.Main or from some network socket). I imagine there could even be a [load:path] directive with which a user could specify an assembly of more directive types, without the application explicitly supporting them; although this would need design for security and for compatibility with unmanaged applications.

@jonsequitur
Copy link
Contributor

I feel that directives should not be specific to a command, not even to a RootCommand. Command-specific stuff should be options instead.

Agreed. The idea of hanging directives off of RootCommand is related to a discussion about whether RootCommand needs to exist at all, beyond just being the root object in the symbol tree. But in terms of the CLI syntax, directives are intended to be "app-level" concepts, and they can only occur before all other tokens. This is why in terms of the parser's structure, it makes sense to only allow this symbol type to be added at the root.

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 a pull request may close this issue.

3 participants