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.

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

Jump to bottom

Add support for generating a bash completion script #107

Merged
merged 6 commits into from
Apr 10, 2020
Merged

Add support for generating a bash completion script #107

merged 6 commits into from
Apr 10, 2020

Conversation

d-e-s-o
Copy link
Owner

@d-e-s-o d-e-s-o commented Apr 5, 2020

This pull request adds support for generating a bash completion script. If
sourced, the shell will provide tab completions for the program's
arguments.
There are two possible approaches provided by clap for going about
generating shell completion functionality: either at build time by
separately generating the clap parsers out-of-band or at run time, as an
option to the main program itself. We are generally not too much in
favor of a run time approach, as it means less inspectability at
installation time and more overhead in the form of code crammed into the
main binary.
Hence, with this change we take the "build time" approach. Clap
recommends hooking the generation up in build.rs, but this seems like an
inflexible choice. For one, that is because it would mean
unconditionally generating this file or some user-unfriendly environment
variable based approach to making the process conditional. But also
because specifying the command for which to generate the script should
likely be configurable. That is a limitation of the completion script
that clap generates (see clap-rs/clap#1764).
Instead, we provide a utility program that emits the completion script
to standard output, accepting regular command line options itself. In
so doing we allow for installation time generation of the completion
script or installation of the utility itself, the output of which could
be sourced on demand -- depending on the user's preference.

@robinkrahl If you have the time, can you please take a look? I think we could now think about consolidating args.rs and commands.rs. Not sure.

@d-e-s-o
Factor out arg_defs.rs for argument parsing related types
728b4f7 
This change marks the first step in a restructuring of the argument
handling code, the ultimate goal of which is a separation of the type
definitions as used by structopt from the logic associated with it. This
change in particular introduces a new module, arg_defs, that contains
all those type definitions that previously resided in the args module.
@d-e-s-o
Move PinType type into arg_defs module
39c1f2b 
The PinType struct, despite being intended for the pinentry module, is
ultimately part of the argument handling definitions. Because our goal
is to consolidate all of those in a single file, this change moves the
definition of this type from the pinentry module into the newly
introduced arg_defs.
@d-e-s-o
Remove dependency on crate::Error from arg_defs
4df0eca 
This change removes the need to import crate::Error from the arg_defs
module. By dropping this dependency we make the file more independent of
the rest of the crate, which subsequently will allow us to merely
include! it in another file in order to get the argument related type
definitions without compilation errors due to missing symbols from the
rest of the crate.
@d-e-s-o
Move dependencies to args and commands modules into generated code
ded1746 
We are aiming to rid the arg_defs module of dependencies to the rest of
the crate in an attempt to make the file fully free standing. The last
remaining references into the crate are used to hook up the functionality
backing the respective commands. Luckily for us, this "gluing" of
functionality to types is really only required in the macro-generated
code (which we do not care about as part of this exercise) and so with
this change we remove the use declarations from the top of the file and
reference the respective functionality in an absolute manner instead.
@d-e-s-o
Add support for generating a bash completion script
d50f84c 
This change adds support for generating a bash completion script. If
sourced, the shell will provide tab completions for the program's
arguments.
There are two possible approaches provided by clap for going about
generating shell completion functionality: either at build time, by
separately generating the clap parsers out-of-band, or at run time, as
an option to the main program itself. We are generally not too much in
favor of a run time approach, as it means less inspectability at
installation time and more overhead in the form of code crammed into the
main binary.
Hence, with this change we take the "build time" approach. Clap
recommends hooking the generation up in build.rs, but this seems like an
inflexible choice. For one, that is because it would mean
unconditionally generating this file or using some user-unfriendly
environment variable based approach for making the process conditional.
But there is also the fact that specifying the command for which to
generate the script should likely be configurable. That is a limitation
of the completion script that clap generates (see
clap-rs/clap#1764).
In our version we provide a utility program that emits the completion
script to standard output, accepting regular command line options
itself. In doing so we allow for installation time generation of the
completion script or installation of the utility itself, the output of
which could be sourced on demand -- depending on the user's preference.
@d-e-s-o
Add test for bash completion functionality
3ec7a91 
This change adds a test for the previously introduced bash completion
functionality. To test the generated completion script, we spin up a
bash instance, source the script, and then perform a completion as the
shell would do it. It seems impossible to convince compgen to do the
heavy lifting for us and so we invoke the completion function with the
expected environment variables present.
@d-e-s-o d-e-s-o force-pushed the topic/bash-complete branch 2 times, most recently from d65bd4e to b6b24f4 Compare April 7, 2020 04:42
@robinkrahl
Copy link
Collaborator

Nice, thank you!

Some minor suggestions:

  • shell-complete --help should mention that it prints the completion script to stdout.
  • I’d prefer a more specific binary name, for example nitrocli-shell-complete.

I would also support merging args.rs into commands.rs (maybe except for handle_arguments which could also go into main.rs). arg_defs.rs could then become args.rs.

@d-e-s-o
Copy link
Owner Author

d-e-s-o commented Apr 10, 2020

Thanks for your review!

  • shell-complete --help should mention that it prints the completion script to stdout.

Done.

  • I’d prefer a more specific binary name, for example nitrocli-shell-complete.

I'd rather not prefix something in the nitrocli repository with nitrocli itself. It seems like useless repetition. I'd say that if users/distributors require a more unique name, they can always rename at installation time.

I would also support merging args.rs into commands.rs (maybe except for handle_arguments which could also go into main.rs). arg_defs.rs could then become args.rs.

Sounds good. Let me do that as a follow up, though.

@d-e-s-o d-e-s-o merged commit 3ec7a91 into devel Apr 10, 2020
@d-e-s-o d-e-s-o deleted the topic/bash-complete branch April 10, 2020 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@d-e-s-o @robinkrahl