fix: support asdf shim-versions completions in fish & bash

[MageJohn]

Summary

I noticed that fish didn't have completion for the shim-versions subcommand, so I went to add it. While doing so I also added the command to the bash completion.

While testing the completion I noticed that my ls alias (which uses exa and adds icons) was being picked up by the completion system, breaking it; I've fixed that too.

Finally, the bash completion for which and shim-versions suggested all possible commands on the system, rather than just the shims like the completion in other shells, so I've changed that.

[hyperupcall]

Thank you for your contribution! I have added some comments to the code.

[MageJohn]

@hyperupcall thanks for checking out my PR! Unfortunately I can't see any comments, I could just be missing something however.

[hyperupcall]

@MageJohn Under Files Changed or if you scroll up on this issue, you should be able to see them? If you don't then you might want to switch to the official client or view this pull request directly on github.com

[hyperupcall]

On my computer, basename seems to complain with "extra operand" errors if there are multiple path arguments and the flag -a or --multiple are not passed. I'm not sure how common these flags are on different platforms

[hyperupcall]

There also appears to be an issue with nullglobs. If no files are found in /shims, basename prints a "missing operand" error.

[MageJohn]

Nice catch on these. I did a little research on basename and it looks like support for a -a flag does vary by system, so to be safe I've used a loop. I've also fixed the empty glob issue by enabling the nullglob option (in a subshell to avoid contaminating the users environment).

[hyperupcall]

Would it be possible to use Fish's path builtin here? At first glance path basename doesn't seem to have the same problems as /usr/bin/basename

[hyperupcall]

There also appears to be an issue with nullglobs. If no files are found in /shims, basename prints a "missing operand" error. In Fish, you can work around this by setting the out to a variable first set files $asdf_data_dir/shims/*, then using files.

[MageJohn]

Nice one, I hadn't noticed the path builtin yet. Using path basename solves the nullglob issue as well.

One thing to note is that the path builtin is relatively new, introduced in version 3.5 last year. That said, fish is usually always installed by users rather than being part of the OS, which means it's more likely to be up to date. Add that to the fact that completions are non-critical for asdf to function, I think it's safe for now.

[hyperupcall]

That's a good point - we're still figuring out which versions of which shells to support in #1436. 3.5 is available on Fedora 38 and 37, but not Debian 11 Bullseye or Ubuntu 22.04. @jthegedus might have a better idea about the versions of Fish we have historically supported

[MageJohn]

While using path basename in this situation is convenient, there's no reason a loop (like for the bash implementation) wouldn't work if the decision is that older fish versions need to be supported. @jthegedus let me know what your decision is, and if needs be I'll happily change the implementation.

[hyperupcall]

Thank you - it was a mistake on my side 🤦

I started a review and I forgot to "submit it" 😛

[MageJohn]

Nice, I can see the comments now! Thanks for the review, I've addressed the feedback.

[hyperupcall]

LGTM! I don't have permission to run the workflows or merge, but @jthegedus can and he can tell us if we support Fish versions below 3.5 or not.

[jthegedus]

can tell us if we support Fish versions below 3.5 or not.

We now target Fish v3.6.1 in our CICD pipeline, as per

asdf/scripts/install_dependencies.bash

Line 25 in d8ce353

fish_semver="3.6.1"

Not sure if we should be testing against multiple shell versions.

And just because we test against one version does not mean it is the minimum supported version (something I would like to track in #1435 as well)

---

My understanding is that since Fish is pre 1.0.0 that most people are updating to the latest as soon as they're aware it's available. Probably a bad assumption if people are using Homebrew 🤷

As pointed out, we can certainly support an older version with more code, but ideally we would comment such code to signal to future contributors that it is specifically to support older versions. I would prefer we just merge this and then if older version users have problems, retrofit the code with comments then.

[jthegedus]

Thanks! As per other comments, we can make specific changes to support older versions should that be a problem for any users unwilling to update to the latest Fish version.