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

RFC-ish: dymanic completions: test a hack that checks availability of generated completions #5376

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ilyagr
Copy link
Contributor

@ilyagr ilyagr commented Jan 16, 2025

Cc @senekor. This is also a bit of an RFC, to make people aware of the interaction
between Fish 4.0 and jj's dynamic completions and my proposed (imperfect) solution
to it, fish-shell/fish-shell#11046 .

In short, unless configured carefully, fish often evaluates the static completions after the dynamic ones, breaking the latter subtly.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@ilyagr ilyagr marked this pull request as ready for review January 16, 2025 00:50
@ilyagr ilyagr changed the title RFC-ish: dymanic completions: test a hack to test availability of generated completions RFC-ish: dymanic completions: test a hack that checks availability of generated completions Jan 16, 2025
@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 16, 2025

I was going to mention that I realized this would be a problem for people using packaged jj that installs something like jj util completion fish | source as a shell completion, but I checked Nixpkgs and brew, and they already switched to dynamic completions. See e.g. NixOS/nixpkgs@ff688dd .

Because of how fish handles completions, if jj is packaged like that, it should work the same on old and new fish.

Of course, this could still be a problem for packagers other than nixpkgs and brew.

@senekor
Copy link
Contributor

senekor commented Jan 16, 2025

I tend towards making jj util completion fish output the string COMPLETE=fish jj | source. I've been using the dynamic completions with fish heavily and have encountered only a tiny issue (which is now fixed in clap).

@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 17, 2025

I tend towards making jj util completion fish output the string COMPLETE=fish jj | source

My current understanding is that this would be fine for fish, but might not be OK for bash and/or zsh as they might run source <(COMPLETE=bash jj | source) on every completion. I'm not sure whether this is a real problem, but if it is, what should we do about jj util completion bash? Is it OK for it to be fundamentally different from jj util completion fish? I'm unsure.

See also fish-shell/fish-shell#11046 (comment) where I said much the same thing.

@senekor
Copy link
Contributor

senekor commented Jan 17, 2025

Is it OK for it to be fundamentally different from jj util completion fish?

I think so, yeah. From the user's perspective, the output of jj util complete is a black box, an implementation detail. All that matters for them is that if they feed it to their shell, they get completions. For each shell, we can choose the output that works best. For bash, that may be static completions. For fish, it's the dynamic ones.

@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 17, 2025

There's one more problem with the idea of having jj util completion fish print COMPLETE=fish jj | source: jj might not be in the PATH and the binary might not even be called jj. One scenario is somebody who already has an older jj installed on their system downloading a newer version of jj to try out.

This could be solved by using the full path, e.g. outputting COMPLETE=fish /full/path/to/jj | source1. I think it's a pretty good default for users. That's not so good for packagers, though, who might be running jj util completion fish before installing jj in the final path it'll live at.

We could say that packagers have to call jj util completion fish --call-jj-as jj instead, but it might be a bit confusing. Also, this makes jj util completion fish more different from jj util compeltion bash.

Footnotes

  1. This is similar to what clap does. E.g. COMPLETE=fish jj-darwin-aarch64 results in complete --keep-order --exclusive --command jj --arguments "(COMPLETE=fish "'/Users/ilyagr/.local/bin/jj-darwin-aarch64'" -- (commandline --current-process --tokenize --cut-at-cursor) (commandline --current-token))" if the jj binary is called that.

@senekor
Copy link
Contributor

senekor commented Jan 17, 2025

jj might not be in the PATH and the binary might not even be called jj

Then the whole process would already fail at jj util completion fish, right? That already requires jj to be in the PATH.

the binary might not even be called jj

I am not aware of a shell completion system that is capable of dealing with users arbitrarily renaming their binaries. I've noticed that in your example ( COMPLETE=fish jj-darwin-aarch64 results in complete --keep-order --exclusive --command jj -- ... ) the command being completed is still --command jj. Have you actually confirmed that jj-darwin-aarch64 is completed correctly this way? How is fish supposed to know to run these completions? You'd have to put them in a different file as well, right? (e.g. ~/.config/fish/completions/jj-darwin-aarch64.fish)

One scenario is somebody who already has an older jj installed on their system downloading a newer version of jj to try out.

I've never heard of people renaming binaries in order to keep around and use multiple ones at the same time. I think it would be way more common to have a certain, possibly somewhat outdated version installed by some package manager and installing a newer one manually. One would generally do that by having ~/.local/bin appear in your PATH before the locations managed by the package manager. Then you can (temporarily) shadow the older jj version without having to rename any binaries.

Also, the static completions currently emitted by jj util completion fish include complete -c jj, which also resolves the path of the jj binary dynamically. I still don't see the problem with that, but even if there is any, I'm pretty sure it's orthogonal to whether jj util completion fish outputs the static or the dynamic completions.

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