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

completion: add branch name completions for fish #3718

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

Conversation

senekor
Copy link
Contributor

@senekor senekor commented May 19, 2024

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

This is related to #1217, but that issue should probably stay open until someone adds these completions for other shells as well.

@ilyagr ilyagr requested a review from bnjmnt4n May 20, 2024 03:35
@ilyagr
Copy link
Collaborator

ilyagr commented May 20, 2024

Cc @bnjmnt4n who made https://gist.github.com/bnjmnt4n/9f47082b8b6e6ed2b2a805a1516090c8.

Not necessarily for this PR, but I wonder whether we could test this functionality in some way.

For example, there could be a test that checks whether fish is installed and then calls it. In CI, we could require that the test pass. (This does seem like it would take some work, maybe I'll look into it one day).

Self::Bash => {}
Self::Elvish => {}
Self::Fish => buf.write_all(CUSTOM_FISH_COMPLETIONS).unwrap(),
Self::Nushell => {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor & optional: Perhaps just use _ => {} syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, I tried that at first. But clippy warns about a single match arm, suggestingif let instead. Adding an allow annotation would be possible too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if let is fine too. Or just leave it be (until clippy gets smart enough to demand converting this code to if let as well).

@senekor
Copy link
Contributor Author

senekor commented May 20, 2024

Oh wow, that's so much better. We should just include that instead of my paltry branch completions. I'll be happy to close this PR and let @bnjmnt4n add their completions instead.

@bnjmnt4n
Copy link
Collaborator

I've made some further tweaks to that script actually, which I hope to update on the Gist soon. It's just that I'm not sure if we want to shift the maintenance of that script into the repo? If it's included in the repo, people's quality expectations are probably higher, and right now the script is kind of cobbled together. As mentioned, testing is likely going to be very difficult (here's fish's own tests for reference: https://github.com/fish-shell/fish-shell/blob/3374692b9113e85c690059ac3def5f4aa190294f/tests/pexpects/complete.py)

I'm happy to defer to the opinions of other contributors here.

@senekor
Copy link
Contributor Author

senekor commented May 20, 2024

Maybe we can find a reasonably simple and robust subset of these possible hand-written completions that can be added upstream? I'd love to see the most recent version of that script.

Alternative idea: Add documentation somewhere pointing people to "community-maintained" scripts. There would be no expectation of quality (they may still live in this repo for discoverability).

@bnjmnt4n
Copy link
Collaborator

Yeah, my script was actually listed in the wiki, which includes other resources as well. Relevant comment: #3711 (comment)

@senekor
Copy link
Contributor Author

senekor commented May 20, 2024

Great, I wasn't aware of that wiki page.

@ilyagr
Copy link
Collaborator

ilyagr commented May 20, 2024

As mentioned, testing is likely going to be very difficult (here's fish's own tests for reference: https://github.com/fish-shell/fish-shell/blob/3374692b9113e85c690059ac3def5f4aa190294f/tests/pexpects/complete.py)

Perhaps something closer to https://github.com/fish-shell/fish-shell/blob/3374692b9113e85c690059ac3def5f4aa190294f/tests/checks/git.fish would work.

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.

None yet

3 participants