Add shell completion supported by clap

[DrSensor]

Which issue does this fix?

Closes #18

Describe the solution

Inspired from starship [1], this PR will add init subcommand to generate shell completion via clap crate [2].

[1] https://starship.rs/#quick-install (jump to step 2)
[2] https://docs.rs/clap/2.33.0/clap/struct.App.html#examples-48

How to test

hub pr checkout 42
cargo install --path .

# if using bash
eval "$(mask init bash)"

As for me, I'm using fish shell and it works

mask init fish | source

Types of changes

• Bug fix
• New feature
• Breaking change

[jacobdeichert]

Hey @DrSensor thanks for the PR! I'll review this week as I have some time off 👍 .

[jacobdeichert]

Thanks again for this PR! It's awesome that clap has support for completions built-in.

So i've been thinking about the init command. I really want to avoid creating subcommands for mask that could conflict with a user's commands.

I was thinking, maybe we do this mask --completion bash|fish|etc instead? Using an option instead of a command aligns more with another existing pattern we have (mask --maskfile path).

Here's how the --maskfile is added https://github.com/jakedeichert/mask/blob/master/src/main.rs#L74-L85

My thinking here is that root level options mask --whatever are ideal for mask-specific logic. If we can stick to this pattern, that would be ideal.

[jacobdeichert]

See comment ^

[mrmurphy]

Wow, would love to see this get merged <3 @DrSensor or @jakedeichert got a moment to take a look? Wish I knew any Rust so I could do it myself :D

[DrSensor]

Ah, thanks @mrmurphy for bringing this up.
I agree with the --completion flag and I think it should render all commands invalid.
Summary, it should error when user do mask --completion=bash command.

I just realized there is one problem with this feature. It can't be added in the startup script ( e.g .config.fish or .bash_profile ).
User can only register the completion manually on the same dir as maskfile.md ( in my case eval (mask --completion fish) ).

[mrmurphy]

Ah okay, that would be a problem!

[jacobdeichert]

I just realized there is one problem with this feature. It can't be added in the startup script ( e.g .config.fish or .bash_profile ).

User can only register the completion manually on the same dir as maskfile.md ( in my case eval (mask --completion fish) ).

Hmmm... wouldn't a user be able to do mask --maskfile ~/global.md --completion bash and pop this into their .bash_profile?

[DrSensor]

Global maskfile should work but not with the local one.
(I wonder how version/workspace manager like pyenv, nvm, asdf can detect and auto-switch based on the local config 🤔)

[jacobdeichert]

Well i guess i was assuming the user would run mask --completion bash | source once inside their project/local directory and then their current shell would have the proper completions available. It would have to be a manual step.

Though, I myself would probably make a helper function or alias in my bash_profile that would wrap mask to auto run this depending on which directory i'm in. But this part is definitely not mask's responsibility... user's should decide what's the best behavior for them, depending on which shell they use.

In the original issue, jgroom posted a bash completions example which involved no changes to mask. I'm now wondering if we should just rely on community-supplied scripts that do simple parsing of the markdown headers for command names. I'd be very happy to link to these scripts (fish, bash, zsh, whatever) from the repo's readme.

If we're unsure about this solution today, we can close this PR and revisit this in the future. Baking completions support into mask i'm indifferent about, currently.

[DrSensor]

I actually in favor of community-supplied scripts though it has the downside that the user need to make sure some built-ins CLI like sed/grep are indeed sed/grep (or at least being aliased properly).
But after fiddling around, I've found a way to generate completion script selectively. This allows

mask --maskfile ~/global.md --completion bash subcmd subsubcmd ...

which generates the completion-script on certain sub/subsub...command. Here I can see a use case when the user needs different auto-completion when opening certain IDE.

Well, supporting both clap and community-supplied scripts is also an option 😄

[jacobdeichert]

@DrSensor thanks for updating the PR and all the work you've done so far!

Unfortunately, I'm going to make the call here to reject this again 🙁

In your last comment, you mentioned that you can now get completions specifically on subcmds and subsubcmds:

mask --maskfile ~/global.md --completion bash subcmd subsubcmd ...

I personally don't think we need to go that far. I think it's an interesting idea, but I don't want to support the extra complexity this contains. I much prefer the simplicity of the user just running mask --maskfile ~/global.md --completion bash | source or mask --completion bash | source depending which mask context they're working in. I know i would be making shortcut aliases for these so it would automatically happen depending on which directory i'm in.

---

I see 2 ways forward from here:

1. We make this PR dead simple and only add support for a --completion sh arg. No cmd filtering or other logic.
2. We close this PR and wait until clap v3 releases to see if they still have completion generation support built in. In the meantime, we link to community supplied scripts instead.

As we both know, mask is a very weird binary. It behaves completely differently based on your context and I'm worried that bundling completion generation now might not result in a great user experience 🙁

[DrSensor]

I choose option 2 then. It seems quite reasonable to wait for clap v3. In my last changes, it's weird that I need to call build_subcommands twice/3-times. Hope clap v3 open the possibility for refactoring mask so that it's easy for adding additional features 🥺

[DrSensor]

Side note for future references.

Even without subcommand filtering, AppSettings::{SubcommandRequired,SubcommandRequiredElseHelp} still need to be disabled or unset_setting