completions/jj: use dynamic completions by default, also fix them#11046
completions/jj: use dynamic completions by default, also fix them#11046krobelus merged 1 commit intofish-shell:masterfrom
Conversation
cc4ae67 to
076f9ee
Compare
d6c1b53 to
f7174db
Compare
|
There are two other options that come to mind. I'll mark this as a draft until I figure out how viable option 2 is. I'd love to resolve this between fish 4 is released (if possible, of course). The two other options I see are:
|
|
After reading clap-rs/clap#5668, it seems like Clap would not recommend using the same command for static and dynamic completions, since they do not want distributions to package the output of So, I'm back to thinking that this PR is the best we can do. If it has a good chance to be accepted, I'll try adding a test to Update: Option 3Yet another option (call it "Option 3") I don't really like would be to change jj's docs. We could instruct people to enable dynamic completions for fish by running # In fish 4.0beta and later, follow these instructions exactly or the
# completions will be subtly broken.
mkdir -p $__fish_config_path/completions
echo 'COMPLETE=fish jj | source' > $__fish_config_path/completions/jj.fishAgain, this doesn't seem great to me. It would also only work if |
…mpletions fish-shell/fish-shell#11046 relies on this behavior.
…d completions fish-shell/fish-shell#11046 relies on this behavior.
share/completions/jj.fish
Outdated
| # https://jj-vcs.github.io/jj/latest/config/#default-command | ||
| if set -l completion (COMPLETE=fish jj __this-command-does-not-exist 2>/dev/null) | ||
| # jj is new enough for dynamic completions to be implemented | ||
| echo "$completion" | source |
There was a problem hiding this comment.
sure this seems like the right solution -- assuming "dynamic" completions are a superset of "static" completions.
We'll take whatever is a stable way to get all completions. Looks fine as is.
Keeping it seems valuable since otherwise adding completions requires manual steps for the user.
(Either way we need to make sure that COMPLETE=fish jj __this-command-does-not-exist always outputs safe fish for any relevant fish version.)
I did not yet have a chance to test dynamic completions but I saw from
$ COMPLETE=fish jj asdf
complete --keep-order --exclusive --command jj --arguments "(COMPLETE=fish "'jj'" -- (commandline --current-process --tokenize --cut-at-cursor) (commandline --current-token))"
That it uses --tokenize; this should be updated to --tokens-expanded (see #10212),
else completions for jj --repository=$HOME/my-repo ... will not work -- unless jj is overstepping its boundaries by expanding shell syntax
(not a blocker; it's probably not a regression)
There was a problem hiding this comment.
This is coming from clap_complete. I doubt it can adopt the new option, since it probably wants to stay compatible with older versions of fish. Once clap_complete does decide to adopt this, jj will automatically get it.
There was a problem hiding this comment.
yes it can, with feature testing as suggested in that link:
set -l tokens
if commandline -x >/dev/null 2>&1
set tokens (commandline -xpc)
else
set tokens (commandline -opc)
endI'll send a patch to clap eventually (unless someone else does it first)
There was a problem hiding this comment.
assuming "dynamic" completions are a superset of "static" completions.
Yes, they are.
| # is not implemented, we'd like to get an error reliably. However, the | ||
| # behavior of `jj` without arguments depends on the value of a config, see | ||
| # https://jj-vcs.github.io/jj/latest/config/#default-command | ||
| if set -l completion (COMPLETE=fish jj __this-command-does-not-exist 2>/dev/null) |
There was a problem hiding this comment.
jj instructions should probably state that it's only necessary for fish below version 4.
Else some users would load duplicate completion definitions (at least Git completions can be slow).
Could make it say something like if [ -z "$(complete -C'jj ')" ]; COMPLETE=fish jj | source; end
which does nothing if completions are already installed
Why do you think That would also solve the problem that packagers shouldn't include the output of |
|
After a quick look at clap-rs/clap#5668, I think the main issue there is that other shells don't do lazy loading of completions. This is not a problem for fish. We can just drop the literal string |
yeah a stable invocation would be preferrable, Seems there is no reason to change the interface
yeah, lazy loaded as soon as completions for jj are requested |
This uses jj's dynamic completions when possible. This avoids an annoying problem. After 04a4e5c, jj's dynamic completions (see the second paragraph of <https://jj-vcs.github.io/jj/latest/install-and-setup/#command-line-completion>) do not work very well in fish if the user puts `COMPLETE=fish jj | source` in their `~/.config/fish/config.fish`. When the user types `jj <TAB>`, they are instead overridden by fish's built-in non-dynamic completions. The difference is subtle. One problem I saw is that `jj new <TAB>` works as expected (and shows revisions) while `jj new -A <TAB>` becomes broken (and shows files). If the user puts `COMPLETE=fish jj | source` in `~/.config/fish/completions/jj.fish` there is no problem. However, users might be confused if they run `COMPLETE=fish jj | source` or put it in their config and it works in a broken fashion. I certainly was. Meanwhile, I checked that if the user has `jj completion fish | source` in their `config.fish`, executing `COMPLETE=fish jj __this_command_does_not_exist | source` afterwards still works correctly. Let me know if there's a better approach to this problem.
|
I fixed up krobelus's comments, thanks for catching my bugs!
I agree. For I'm not sure whether it makes sense for Aside: As discussed on Discord, nixpkgs already does the analogous (slow) thing for bash and zsh: nixpkgs. I think brew caches the output of Another aside: perhaps we could implement feature detection in |
| # https://jj-vcs.github.io/jj/latest/config/#default-command | ||
| if set -l completion (COMPLETE=fish jj __this-command-does-not-exist 2>/dev/null) | ||
| # jj is new enough for dynamic completions to be implemented | ||
| printf %s\n $completion | source |
There was a problem hiding this comment.
Another issue is that computing completions purely inside jj means that
it won't work if there are shell metacharacters. Examples:
jj --repository ~/jj --repository $HOME/
These don't get any completions. Note that commandline --tokens-expanded won't
work because (without a heuristic) fish doesn't know how to apply /home/user/foo non-destructively.
The currently recommended approach is to delegate back to fish's file completions if
jj finds that we are completing a file path.
Normally file completions are disabled via complete --exclusive but we can do something like
function __fish_jj_complete
set -g __fish_jj_completions (COMPLETE=fish jj -- (commandline --current-process --tokenize-expanded --cut-at-cursor) (commandline --current-token))
test $status -ne 5 # status 5 means "fall back to file completions"
end
complete jj --keep-order --exclusive --command jj \
--condition __fish_jj_complete --arguments '(string join \n $__fish_jj_completions)'I guess in this specific case we wouldn't even fallback but instead compute directory completions, using the existing:
complete -c jj -n "__fish_jj_needs_command" -s R -l repository -d 'Path to repository to operate on' -r -f -a "(__fish_complete_directories)"
This is why hand-written completions are often better - but of course we can fix them (probably needs another change in Clap).
I guess this is not blocking, because using variables in file paths is somewhat rare for jj (given that
most file paths for jj diff and others get completed just fine). But we should fix that.
There was a problem hiding this comment.
Note that
commandline --tokens-expandedwon't
work because (without a heuristic) fish doesn't know how to apply/home/user/foonon-destructively.
I don't understand this, but my fish version doesn't support that option yet, so I can't experiment with it. What does --tokens-expanded do exactly? Why doesn't it expand jj --repository ~/ to /home/user/ before passing it to clap for dynamic completions?
There was a problem hiding this comment.
This is interesting. I think this is far from being specific to jj. I don't immediately see a better solution than the commits you linked to this PR (and I don't fully understand them at the moment either).
I think it will only become more of a problem as more CLI libraries support dynamic completion (I think there's already clap, https://github.com/spf13/cobra , and some Python libraries whose name I forgot1). I hope they will; naively it seems like an ultimately better approach than shipping static completions in shell-and-package-manager dependent ways.
To me, this suggests the need for some more detailed communication protocol between the shell and clap or the like. It's hard for me to decide whether --token-expanded is a sufficient protocol for this. I'm glad it exists. I haven't followed the details, but I'm guessing that it's an 80% solution, and something better will eventually be needed for the other 20% (which will take 500% as much work).
Footnotes
-
I was thinking of https://github.com/kislyuk/argcomplete (which actually supports Fish despite first claiming not to) which works with argparse from the standard library. I think most people use argparse by default? Not sure if something works with the newer Python CLI libraries, or what the fashion of the day is. ↩
There was a problem hiding this comment.
but I'm guessing that it's an 80% solution
yes absolutely. --tokens-expanded was added as an incremental improvement to fix one bad aspect of --tokens
and something better will eventually be needed for the other 20% (which will take 500% as much work).
--tokens-expanded works fine for getting the list of tokens before the token at cursor.
This list can be fed into a completion generator that has no idea of shell syntax.
Some remaining problems are:
- it breaks on multiline tokens (and tokens that expand to multiple lines) -- should probably add a
--zerooption - when expansion would produce too many values, we pass on the unexpanded string. This is hacky but unlikely to cause problems in practice
- The most difficult one:
--tokens-expandedonly works for tokens to the left, not for the token at cursor.
To fix this we indeed need some kind of communication protocol for passing arbitrary metadata.
Considermyapp {foo,where
the shell would need to tell the completion generator that it should generate something based offmyapp foo
Also givenmyapp $HOME/, the shell would ask for completions based on the expanded$HOME, but would want to un-expand the variable before applying the completion.
I think it's unrealistic that every completion client implements their own fish tokenizer.
All those completions like ls {foo, already work in fish, so we could get there by plugging this into external completions.
IME apps providing completion generation are reluctant to use wildly different code for fish than for bash/zsh,
so I'd love to find an interface that can be easily implemented in any shell.
That will also be a soft prerequisite for this thing to be adopted (fish is small enough to have self-interest in not being evil ;).
| # is not implemented, we'd like to get an error reliably. However, the | ||
| # behavior of `jj` without arguments depends on the value of a config, see | ||
| # https://jj-vcs.github.io/jj/latest/config/#default-command | ||
| if set -l completion (COMPLETE=fish jj __this-command-does-not-exist 2>/dev/null) |
There was a problem hiding this comment.
Using jj 0.23.0-0ca6f00421f6f893e974e0f5349e126abd41a812
Pressing tab on
$ jj --
prints a spurious
Hint: Use jj -h for a list of available commands.
Similar on jj --repository /.
EDIT: Also on jj abandon @ --ignore-im
EDIT2: Missing completions on jj abandon qxnzqwuk:: --ignore-
There was a problem hiding this comment.
AFAICT the only remaining regression is clap-rs/clap#5949
I'm planning to merge this PR after that clap fix is in jj. Though it may be worth merging before that, not sure
In fish, the command line
cargo b --manifest-path $PWD/clap_complete/Cargo.toml --example
wrongly gets example-name completion $PWD/Cargo.toml instead of
$PWD/clap_complete/Cargo.toml.
This is because fish's "commandline --tokenize" option produces tokens
like "$PWD/clap_complete/Cargo.toml", which clap cannot see through.
Starting in the upcoming fish version 4, there is a new option,
"--tokens-expanded"[^1] to output the expanded arguments, matching
what the program would see during execution.
If an expansion of a token like {1,2,3}{1,2,3} would produce too
many results, the unexpanded token is forwarded, thus falling back to
existing behavior. Similarly, command substitutions ("$()") present
on the command line are forwarded as-is, because they are not deemed
safe to expand given that the user hasn't agreed to running them.
Use this option if available, to fix the above example.
While at it, break up this overlong line. Add redundant semicolons,
in case a preexisting completion accidentally joins these lines with
spaces[^2], like
if set -l completions (COMPLETE=fish myapp 2>/dev/null)
echo "$completions" | source
end
I have not updated clap_complete/tests/snapshots/register_dynamic.fish
(not sure how?) or tested this but I'm happy to do so.
[^1]: fish-shell/fish-shell#10212
[^2]: fish-shell/fish-shell#11046 (comment)
In fish, the command line
cargo b --manifest-path $PWD/clap_complete/Cargo.toml --example
wrongly gets example-name completions from $PWD/Cargo.toml instead of
$PWD/clap_complete/Cargo.toml.
This is because fish's "commandline --tokenize" option produces tokens
like "$PWD/clap_complete/Cargo.toml", which clap cannot see through.
Starting in the upcoming fish version 4, there is a new option,
"--tokens-expanded"[^1] to output the expanded arguments, matching
what the program would see during execution.
If an expansion of a token like {1,2,3}{1,2,3} would produce too
many results, the unexpanded token is forwarded, thus falling back to
existing behavior. Similarly, command substitutions ("$()") present
on the command line are forwarded as-is, because they are not deemed
safe to expand given that the user hasn't agreed to running them.
Use this option if available, to fix the above example.
I have not checked if any existing clap user tries to expand variables
on their own. If this is a real possibility, we could let users
opt into the new behavior.
While at it, break up this overlong line. Add redundant semicolons,
in case a preexisting completion accidentally joins these lines with
spaces[^2], like
if set -l completions (COMPLETE=fish myapp 2>/dev/null)
echo "$completions" | source
end
I have not updated clap_complete/tests/snapshots/register_dynamic.fish
(not sure how?) or tested this but I'm happy to do so.
[^1]: fish-shell/fish-shell#10212
[^2]: fish-shell/fish-shell#11046 (comment)
TODO: should double-check the root cause and add automated tests
jj dynamic completions of option names work as expected ...
$ COMPLETE=fish jj -- jj abandon --ignore-
--ignore-working-copy
--ignore-immutable
... unless there is a positional argument
$ COMPLETE=fish jj -- jj abandon @ --ignore-
The positional arguments are defined in the "abandon" subcommand as:
```rust
#[derive(clap::Args, Clone, Debug)]
pub(crate) struct AbandonArgs {
/// The revision(s) to abandon (default: @)
#[arg(
value_name = "REVSETS",
add = ArgValueCandidates::new(complete::mutable_revisions)
)]
revisions_pos: Vec<RevisionArg>,
}
```
After clap_complete sees the first positional argument ("@") we are stuck
in "ParseState::Pos(..)" even when we get to parsing "--ignore-". This is
because "revisions_pos" accepts an arbitrary number of positionals.
While in "ParseState::Pos(..)", we only produce completions for positionals
(via "complete_arg_value()"). This means we fail to produce option-name
completions like "--ignore-immutable".
This was introduced in f0bd475 (feat(clap_complete): Support
multi-values of positional argument with `num_arg(N)`, 2024-07-26).
Presumably, the main intention of that commit was to allow options
with multiple, but finitely many arguments.
For the case where arbitrarily many arguments are allowed, let's get rid of
this restriction, to fix the false negative.
Ref: fish-shell/fish-shell#11046 (comment)
jj dynamic completions of option names work as expected:
$ COMPLETE=fish jj -- jj abandon --ignore-
--ignore-working-copy
--ignore-immutable
Unless there is a positional argument preceding the option
$ COMPLETE=fish jj -- jj abandon @ --ignore-
The positional arguments are defined in the "abandon" subcommand as:
```rust
#[derive(clap::Args, Clone, Debug)]
pub(crate) struct AbandonArgs {
/// The revision(s) to abandon (default: @)
#[arg(value_name = "REVSETS")]
revisions_pos: Vec<RevisionArg>,
}
```
After clap_complete sees the first positional argument ("@") we'll be stuck
in "ParseState::Pos(..)" forever; in particular "--ignore-" will be treated
as positional argument by the clap_complete parser.
This is inconsistent with execution where "--ignore-" will correctly be
treated as option.
I have not yet checked if other scenarios than a derived `Vec<>` are affected.
For now fix the parsing of that one only.
Ref: fish-shell/fish-shell#11046 (comment)
jj dynamic completions of option names work as expected:
$ COMPLETE=fish jj -- jj abandon --ignore-
--ignore-working-copy
--ignore-immutable
Unless there is a positional argument preceding the option
$ COMPLETE=fish jj -- jj abandon @ --ignore-
The positional arguments are defined in the "abandon" subcommand as:
```rust
#[derive(clap::Args, Clone, Debug)]
pub(crate) struct AbandonArgs {
/// The revision(s) to abandon (default: @)
#[arg(value_name = "REVSETS")]
revisions_pos: Vec<RevisionArg>,
}
```
After clap_complete sees the first positional argument ("@") it will be
stuck in "ParseState::Pos(..)" forever; in particular "--ignore-" will be
treated as positional argument by the clap_complete parser.
This is inconsistent with execution where "--ignore-" will correctly be
treated as option, because the "Append" arg action implies that the arguments
need not be contiguous. Fix this inconsistency.
Ref: fish-shell/fish-shell#11046 (comment)
To avoid redundant computation, users who run `COMPLETE=fish jj | source` in their ~/.config/fish/config.fish should eventually remove that, or move it to ~/.config/fish/completions/jj.fish which overrides the default completions. Ref: fish-shell/fish-shell#11046
To avoid redundant computation, users who run `COMPLETE=fish jj | source` in their ~/.config/fish/config.fish should eventually remove that, or move it to ~/.config/fish/completions/jj.fish which overrides the default completions. Ref: fish-shell/fish-shell#11046
jj dynamic completions of option names work as expected:
$ COMPLETE=fish jj -- jj abandon --ignore-
--ignore-working-copy
--ignore-immutable
Unless there is a positional argument preceding the option
$ COMPLETE=fish jj -- jj abandon @ --ignore-
The positional arguments are defined in the "abandon" subcommand as:
```rust
#[derive(clap::Args, Clone, Debug)]
pub(crate) struct AbandonArgs {
/// The revision(s) to abandon (default: @)
#[arg(value_name = "REVSETS")]
revisions_pos: Vec<RevisionArg>,
}
```
After clap_complete sees the first positional argument ("@") it will be
stuck in "ParseState::Pos(..)" forever; in particular "--ignore-" will be
treated as positional argument by the clap_complete parser.
This is inconsistent with execution where "--ignore-" will correctly be
treated as option, because the "Append" arg action implies that the arguments
need not be contiguous. Fix this inconsistency.
Ref: fish-shell/fish-shell#11046 (comment)
Follow-up to #6059, see also fish-shell/fish-shell#11046.
See also the discussion in fish-shell/fish-shell#11046 and discussions linked from there.
This uses jj's dynamic completions when possible.
This avoids an annoying problem. Because of 04a4e5c (so, in fish 4.0beta, but not in fish 3.7.1), jj's dynamic completions (added to jj recently, see the second paragraph of https://jj-vcs.github.io/jj/latest/install-and-setup/#command-line-completion) do not work very well in fish if the user puts
COMPLETE=fish jj | sourcein their~/.config/fish/config.fish. When the user typesjj <TAB>, they are instead overridden by fish's built-in non-dynamic completions.The difference is subtle. One problem I saw is that
jj new <TAB>works as expected (and shows revisions) whilejj new -A <TAB>becomes broken (and shows files).If the user puts
COMPLETE=fish jj | sourcein~/.config/fish/completions/jj.fishthere is no problem. However, users might be confused if they runCOMPLETE=fish jj | sourceor put it in their config and it works in a broken fashion. I certainly was, this took a while to debug.Meanwhile, I checked that if the user has
jj completion fish | sourcein theirconfig.fish, executingCOMPLETE=fish jj __this_command_does_not_exist | sourceafterwards still seems to work correctly. So, this PR should not cause the oppositeproblem to the one it tries to solve.
Let me know if there's a better approach to this problem.
TODOs: