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

bat.fish: There should be only one #651

Closed
adiabatic opened this issue Sep 4, 2019 · 19 comments
Closed

bat.fish: There should be only one #651

adiabatic opened this issue Sep 4, 2019 · 19 comments
Labels
help wanted Extra attention is needed packaging/tooling
Milestone

Comments

@adiabatic
Copy link

adiabatic commented Sep 4, 2019

A while back, during 0.11.x, I thought bat didn't have any fish completion file, so I wrote one. When 0.12.0 came out, I updated the fish completion file to support the freshly-added -L option. This file is in the fish repository, and will be released to the public whenever the fish maintainers get around to releasing a new version (in all likelihood, not too soon).

Today, when watching 0.12.1 come in via Homebrew, I noticed that Homebrew installed a bat completion of its own into /usr/local/share/fish/vendor_completions.d. Seems like I've been scooped.

I figure that the completion in the fish repo should be deleted in favor of the one provided with bat itself. I think the one I wrote has a number of improvements over the one that's bundled with bat:

  • It's in nice-looking columns and is very easy to read, assuming you have a wide-enough monitor. (Multi-cursor editing is a godsend.)
  • It moves the "which is the default" verbiage into the option-completion part of the completion scripts, saving horizontal space and making sure it doesn't get collapsed to a "…".
  • It supports all the bat cache options, not just build and clear.

There are also a couple other differences that I feel I should point out:

  • My options are sorted by the order seen in the --help text. The bat.fish that ships with bat sorts everything by -l value (long option).
  • My __bat_complete_languages_and_extensions assumes that one can specify a language by typing out its fancy name and not just an extension. (Maybe this assumption is wrong.)
  • Mine omits -u/--unbuffered, since it's just a POSIX-compliance thing that gets ignored by bat anyway.

Would bat be better if the completion currently sitting in the fish repository were incorporated into the bat distribution? Regardless of whether the answer is "yes" or "no", I ought to remove the one in the fish repository.

@eth-p
Copy link
Collaborator

eth-p commented Sep 5, 2019

This might need to be discussed a bit. There's a few things that need to be taken into consideration here.

In regards to how the completions are distributed, I think having them distributed in the bat packages would definitely be ideal. This would allow for them to be relevant to user's installed version of bat rather than set in stone until the next fish release. My only lingering concern with this method is how it should be handled when bat and fish have different install prefixes (e.g. if bat is installed to /usr/local while fish is installed to /usr). In such a case, would fish check the other install prefix for bat's vendor completions, or would bat's completions just not be loaded?

I also think that incorporating your completions into the one provided by bat would be a great idea, but there's a couple things that I feel should be discussed:

Which sort order for the option completions would be the best one to use? Each of them have their own advantages. Alphabetical is ordered in a straightforward way, while ordering it in the same way as --help is more consistent. There's also a third option of sorting it based on how frequently used the options are.

Although the -u option exists strictly for compatibility and doesn't actually do anything, should it be omitted from completions? It's functionally useless and takes up space when going through the completions, but it's also a documented option in bat.

@sharkdp, your thoughts?

@adiabatic
Copy link
Author

My only lingering concern with this method is how it should be handled when bat and fish have different install prefixes (e.g. if bat is installed to /usr/local while fish is installed to /usr). In such a case, would fish check the other install prefix for bat's vendor completions, or would bat's completions just not be loaded?

I think the current fish way is to let the packaging people figure that out for themselves. At least on macOS with Homebrew, they do a fine job. On the other hand, FreeBSD doesn't. I should ask on the fish mailing list, really. I don't know if they've figured out best-practice for this, either.

Which sort order for the option completions would be the best one to use? Each of them have their own advantages. Alphabetical is ordered in a straightforward way, while ordering it in the same way as --help is more consistent. There's also a third option of sorting it based on how frequently used the options are.

  • --help/manpage order is easiest for the person who first writes the thing and is reasonably pleasant for someone who's looking to add a new option that's been freshly added in a new version of bat.
  • Alphabetizing it by -l (long option) is sensible if all options have a long option and is also reasonably pleasant if you're looking for the right place to add a new option. That said, I'd still recommend grouping by subcommand (or lack thereof).
  • I am unclear who, if anyone, benefits from turning the fish completions into a most-recently-used list. Certainly not anyone who wants to see if a particular new option has made it into the completions file.

Although the -u option exists strictly for compatibility and doesn't actually do anything, should it be omitted from completions? It's functionally useless and takes up space when going through the completions, but it's also a documented option in bat.

I say it should be omitted from the completions because nobody who's typing out bat options will want to add -u. I wouldn't want fish to suggest a useless option; reading about it takes up time and space that I'd rather devote to options that I might conceivably want to add. This makes complete -c bat -s u -l unbuffered … anti-useful, and not merely useless.

@sharkdp
Copy link
Owner

sharkdp commented Sep 6, 2019

Which sort order for the option completions would be the best one to use? Each of them have their own advantages. Alphabetical is ordered in a straightforward way, while ordering it in the same way as --help is more consistent. There's also a third option of sorting it based on how frequently used the options are.

I made an explicit choice to not sort the -h arguments in alphabetic order, as I wanted them to be in "most useful"/"most frequently used" order. I guess that might not actually be the case any more (IMO, --map-syntax/--list-languages should be lower down, --plain/--number/--show-all should be higher up). We should probably reorder this in the --help text and then use the same order in the fish completions as well.

👍 for removing -u from the completions.

@eth-p
Copy link
Collaborator

eth-p commented Sep 6, 2019

I think the current fish way is to let the packaging people figure that out for themselves. At least on macOS with Homebrew, they do a fine job. On the other hand, FreeBSD doesn't. I should ask on the fish mailing list, really. I don't know if they've figured out best-practice for this, either.

I totally agree that it's fine with letting the packaging people figure it out themselves, since they know their own ecosystems better than we would. I would also like to be able to have the official GitHub release packages to work properly in the majority of cases as well, though.

Should we research this and see if it's possible to create post-install scripts to detect and symlink the completions if the prefix is different than what fish is installed to? Or would that be bad practice? I would be happy to move forward on merging the completions regardless, but if it's something that you guys think is worth pursuing, I can open a new issue on it and we can tackle it later down the line.

--help/manpage order is easiest for the person who first writes the thing and is reasonably pleasant for someone who's looking to add a new option that's been freshly added in a new version of bat.
Alphabetizing it by -l (long option) is sensible if all options have a long option and is also reasonably pleasant if you're looking for the right place to add a new option. That said, I'd still recommend grouping by subcommand (or lack thereof).
I am unclear who, if anyone, benefits from turning the fish completions into a most-recently-used list. Certainly not anyone who wants to see if a particular new option has made it into the completions file.

Seeing as how @sharkdp was already going for the "most useful first" order within --help, then sorting by manpage/--help would definitely be the way to go.

I say [-u] should be omitted from the completions because nobody who's typing out bat options will want to add -u. I wouldn't want fish to suggest a useless option; reading about it takes up time and space that I'd rather devote to options that I might conceivably want to add. This makes complete -c bat -s u -l unbuffered … anti-useful, and not merely useless.

Very good point. It would probably be fairly annoying and unhelpful to have a useless option show up as an actual suggestion when you're trying to find the one you want. I'm all for removing it, then.

@eth-p
Copy link
Collaborator

eth-p commented Sep 27, 2019

When #673 is merged, we should combine @adiabatic's completions with the ones in this repo.

I've also spent some time thinking of how to package the completions. Ideally, package maintainers would add the fish completions to the relevant vendor-completions path for fish and everything would just work. But, we can't exactly guarantee that is always going to be the case.

How about we have two copies of the fish completions? Fish can provide its own copy (possibly outdated, but better than nothing), and we can provide a vendor copy on top of that. Based on my testing with fish 3.0.2, it has a sane load order that works perfectly for this:

  1. user completions
  2. vendor completions
  3. fish provided completions

If there are multiple completion files, it will only read the higher priority one. No mixed/overridden completions.

@adiabatic
Copy link
Author

If there are multiple completion files, it will only read the higher priority one. No mixed/overridden completions.

That's good to hear. When I read about complete --erase I guessed that completions in all the different places would be added together with some sort of per-completion last-completion-standing battle royale. Happy to be wrong about that.

@sharkdp
Copy link
Owner

sharkdp commented Dec 22, 2019

Just a quick note that this is not on my radar. Happy to merge changes that resolve this, though.

@dl1ely
Copy link

dl1ely commented Feb 12, 2020

This seems to be related i guess. I am on Linux Mint (Ubuntu bionic). I installed the bat deb file manually using sudo dpkg -i bat_0.12.1_amd64.deb. Today i got a fish shell update to 3.1.0-1~bionic offered from http://ppa.launchpad.net/fish-shell/release-3/ubuntu/, but this fails with: trying to overwrite '/usr/share/fish/completions/bat.fish', which is also in package bat 0.12.1. I did not tinker with fish completions at all. Obviously, both packages try to be the authoritive source of fish completions. Is there any solution or thoughts on this issue? Thank you.

@ronjouch
Copy link

ronjouch commented Feb 14, 2020

Related discussion: fish-shell/fish-shell#5822 (comment) , where a fish dev suggests that 3rd-parties should package their completions in /usr/share/fish/vendor_completions.d instead of /usr/share/fish/completions which is fish's sole kingdom.

@jswny
Copy link

jswny commented Feb 17, 2020

Just ran into the same problem as @dl1ely. Is there any way to fix this in the meantime? I can't install the .deb package because of this problem.

@sharkdp
Copy link
Owner

sharkdp commented Feb 17, 2020

Not a quickfix, but I guess it would be rather easy to fix with the suggestion by @ronjouch:

install -Dm644 "assets/completions/bat.fish" "$tempdir/usr/share/fish/completions/$PROJECT_NAME.fish"

@dl1ely
Copy link

dl1ely commented Feb 17, 2020

@jswny I got it force-installed using apt -o Dpkg::Options::="--force-overwrite" -f upgrade IIRC. Got that from some google search result.

@jswny
Copy link

jswny commented Feb 17, 2020

@dl1ely Thanks, that worked!

@thernstig
Copy link

I'd expect an influx of users over time that experience this problem. I just experienced this problem myself. If you are a fish user, you are also often "cool enough" to use awesome tools such as bat and ripgrep. Both now fails to install under the latest fish shell. ripgrep already pushed a fixed for this (see BurntSushi/ripgrep@bb49e6f#diff-80398c5faae3c069e4e6aa2ed11b28c0R104) but it has not been released yet.

I think this breaking bat installation for fish shell users warrants bat to do a patch version upgrade with a fix. The fix itself is simple to implement, but I am unsure about bat's release process.

@zanchey
Copy link

zanchey commented Feb 22, 2020

Don't force install; Debian gives you the right tool for the job with dpkg-divert:

dpkg-divert --add --divert  /usr/share/fish/completions/bat.fish.0 --rename --package bat /usr/share/fish/completions/bat.fish

I'll get fish 3.1.1 out the door soon, which will also fix the problem.

@dl1ely
Copy link

dl1ely commented Feb 22, 2020

@zanchey Perhaps i did something wrong, i tried the dpkg-divert, but it did not change anything. The error message stayed the some on subsequent installation tries. Perhaps i got the parameters wrong. If this works, it surely is the superior solution.

@zanchey
Copy link

zanchey commented Feb 22, 2020

If you have used force-install, you probably have to remove bat, fish-common, and fish, and reinstall them all.

@sharkdp
Copy link
Owner

sharkdp commented Feb 22, 2020

I think this breaking bat installation for fish shell users warrants bat to do a patch version upgrade with a fix. The fix itself is simple to implement, but I am unsure about bat's release process.

Happy to release a new version once this is fixed.

@sharkdp sharkdp added this to the v0.13 milestone Mar 6, 2020
@sharkdp sharkdp closed this as completed in 61e3915 Mar 6, 2020
@sharkdp
Copy link
Owner

sharkdp commented Mar 22, 2020

This has been fixed in bat 0.13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed packaging/tooling
Projects
None yet
Development

No branches or pull requests

8 participants