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

Make bash completion more flexible #1765

Closed
wants to merge 1 commit into from
Closed

Make bash completion more flexible #1765

wants to merge 1 commit into from

Conversation

d-e-s-o
Copy link

@d-e-s-o d-e-s-o commented Mar 26, 2020

The bash completion, as it is generated by clap, is specific to the
command it is created for. E.g., that is, if it was created for 'myapp'
it can only ever be used for completion on the 'myapp' command. This
constraint is rooted in said command (being provided at build time)
being embedded in the generated code. While it is sensible to enable
completion for the supplied command by default, there should be a way to
make it apply to other commands as well (think, bash aliases, for
example).
Completions for other tools such as git support this out of the box via
an indirection: by providing a function that registers the completion
and that accepts the command to register it for.
With this change we enable this use case for the clap generated bash
completion as well. Instead of hard coding the command name we expect,
we rely on the first argument provided by the complete built-in, which
evaluates to exactly this command (see bash(1); ctrl-f '-F function').
The completion for myapp as it occurs when sourcing the completion
script is unaffected.
With this change, a completion generated for 'myapp' can now be
repurposed to work with a hypothetical alias 'app' as follows:

complete -F _myapp -o bashdefault -o default app

Fixes #1764


I am not familar with clap_generate and I need the fix on v2, hence, pull request for v2-master. Change can be crossported to master if desired.

The bash completion, as it is generated by clap, is specific to the
command it is created for. E.g., that is, if it was created for 'myapp'
it can only ever be used for completion on the 'myapp' command. This
constraint is rooted in said command (being provided at build time)
being embedded in the generated code. While it is sensible to enable
completion for the supplied command by default, there should be a way to
make it apply to other commands as well (think, bash aliases, for
example).
Completions for other tools such as git support this out of the box via
an indirection: by providing a function that registers the completion
and that accepts the command to register it for.
With this change we enable this use case for the clap generated bash
completion as well. Instead of hard coding the command name we expect,
we rely on the first argument provided by the complete built-in, which
evaluates to exactly this command (see bash(1); ctrl-f '-F function').
The completion for myapp as it occurs when sourcing the completion
script is unaffected.
With this change, a completion generated for 'myapp' can now be
repurposed to work with a hypothetical alias 'app' as follows:
  > complete -F _myapp -o bashdefault -o default app

Fixes #1764
@pksunkara
Copy link
Member

We will not be releasing v2 anytime soon (only security releases). So, I would recommend changing this to master. A beta release should come out soon.

@d-e-s-o
Copy link
Author

d-e-s-o commented Mar 26, 2020

Yeah, after testing some more it seems the approach works only for the first level. So these additional lines would need to be adjusted as well. That does not seem to be straight forward, though, with the weird logic in there.

In any case, I am not interested in v3 as I only consume clap indirectly and have no influence over when a new major version will be used -- irrespective of when it gets released. So if that change is not landing on v2 then I am not spending more time on it.

@d-e-s-o
Copy link
Author

d-e-s-o commented Mar 27, 2020

Let me close this pull request. I've gone down a slightly different route now to work around this problem. Sorry for the noise.

@d-e-s-o d-e-s-o closed this Mar 27, 2020
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 this pull request may close these issues.

2 participants