feat(cli/rustup-mode): support rustup completion for nushell
#4614
Conversation
rami3l
force-pushed
the
cli/nushell-completion
branch
2 times, most recently
from
ad09574 to
89d74ec
Compare
rami3l
force-pushed
the
cli/nushell-completion
branch
from
89d74ec to
fad89bf
Compare
| } | ||
|
|
||
| #[derive(clap::ValueEnum, Clone, Copy, Debug)] | ||
| enum CompletionShell { |
There was a problem hiding this comment.
It seems pretty unclear from the code why this needs to be a separate enum rather than another variant added to Shell? Should at least clarify in a comment/docstring.
There was a problem hiding this comment.
Shell is an external enum so cannot be extended. Do you have some other ideas regarding the extension of such a type? Will nested enums be better in your opinion?
There was a problem hiding this comment.
@epage why is this a separate crate? Seems like a pain in the ass for downstreams?
There was a problem hiding this comment.
PS: When I say this PR mirrors the jj one that's because jj has also flattened the enum like this.
There was a problem hiding this comment.
why is this a separate crate? Seems like a pain in the ass for downstreams?
There are two distinctions
- In
clap_completeor not - In
Shellor not
Looking back in the past, I wish we had fewer shells in Shell then we do and in the next major version would like to trim it down and locking it down. imo we shouldn't be implicitly extending the compatibility surface of our callers by adding new content to Shell. Pulling in esoteric shells like elvish and fig should be explicit choices. Where the line is for what should be in clap_complete or even in Shell is not yet determined.
See also
- Move clap_complete_fig and clap_complete_nushell into clap_complete clap-rs/clap#5329
- Why are the nushell completions not part of the completions crate? clap-rs/clap#5880
At least with our new completion system, it is easier to add new shells, see https://docs.rs/clap_complete/latest/clap_complete/env/struct.Shells.html
Would be willing to explore something similar for our existing completion system.
There was a problem hiding this comment.
Maybe there shouldn't be an enum at all, only a trait?
3 participants
Mirrors https://github.com/jj-vcs/jj/blob/54511ee3fde4f381a67a38ca3a4a0f1c7412753a/cli/src/commands/util/completion.rs#L80; closes #4603.