feat(command-not-found): Support Nushell #271
Conversation
There was a problem hiding this comment.
thanks! looking good overall, mostly curious about the exit code.
for the record, looks like potential follow-ups would be:
- make this easier to use
- new release
- update the package in nixpkgs, adding the script as in the flake here
- update the home-manager module to abstract out the include with an
enableNushellIntegrationoption
add support for theNIX_AUTO_INSTALL/NIX_AUTO_RUNenvironment variables
| 1 => (do $single_pkg ($pkgs | get 0)), | ||
| _ => (do $multiple_pkgs $pkgs), | ||
| } | ||
| return $ret |
There was a problem hiding this comment.
the bash version uses exit code 127, maybe we should match that here
There was a problem hiding this comment.
But the return value from Nushell's command_not_found is not an exit code. It represents the message to display to the user. I literally started using Nushell a week ago, so please correct me if I'm wrong. But this is what I understood after reading this section from the Nushell book.
| let single_pkg = { |pkg| | ||
| let lines = [ | ||
| $"The program '($cmd_name)' is currently not installed." | ||
| "" |
There was a problem hiding this comment.
not sure this would be worth taking into consideration, but the (minor) formatting differences here could have some unintended effects on command-parsing tools (think thefuck, pay-respects)
There was a problem hiding this comment.
Interesting. I never used such tools so I didn't take that into consideration. Shall I install those tools locally and compare their behavior when using command-not-found.sh vs command-not-found.nu?
There was a problem hiding this comment.
looks like so far upstream/PRs include at least:
- https://codeberg.org/iff/pay-respects/src/commit/60a168e02ca7dc9951e101674f0395d9740d11c7/core/src/system.rs#L183:
result.contains("did you mean") || result.contains("is not installed") - https://github.com/nvbn/thefuck/pull/1393/files#diff-c17d576fdd8077419c4491a0bc945b2e360a157c1860734fa4b518a095663c0bR37:
"command not found" in command.output - https://github.com/nvbn/thefuck/blob/master/thefuck/rules/nixos_cmd_not_found.py:
re.compile(r'nix-env -iA ([^\s]*)')
.. so looks like so far these have been written general enough to not just trip over minor formatting differences (my fear), tho to be fair one may question where the burden lies of keeping their scripts working as well.
to be fair to them, maybe the one parsing for command not found might be easy enough to handle.
as for the old thefuck rule i made parsing for nix-env -iA , i guess i understand you opted to not complicate things here, while i realize flakes (and their nix cli v3 interface) have become fairly popular as well. in practice i think that situation is a bit complicated (most core contributors do not use flakes), tho tbf thefuck is a bit dead, so i'm kinda like whatever.
There was a problem hiding this comment.
I wrote this hook for myself initially and wasn't planning to contribute it. That's why I focused on my specific usecase. Should I change command-not-found.nu to use the v3 interface only if flakes are enabled in nix.conf? That way it would make sense for both flake users and non-flake users.
There was a problem hiding this comment.
yeah, no biggie - it already looks great, the potential follow-ups i suggested i meant more as a note to self than a burden on you.
on flake checks to propose different commands, i think that's what command-not-found.sh was doing with its checks for manifest.json, so if you wanted to add something like that it could make sense to take some inspiration from their implementation.
There was a problem hiding this comment.
The manifest.json check seems to tell us whether the user is using Nix profiles or not. And not whether they enabled flakes or not. However, I never used profiles / manifest.json so I'm kinda lost here. So we have four cases now:
- Flakes disabled / profiles disabled
- Flakes enabled / profiles disabled
- Flakes disabled / profiles enabled
- Flakes enabled / profiles enabled
There was a problem hiding this comment.
looks like my earlier comments w.r.t. status code and NIX_AUTO_INSTALL / NIX_AUTO_RUN seem not applicable in the Nushell context, as @ameertaweel noted.
@bennofs @figsoda @RaitoBezarius would you mind reviewing as well? 🙏 ✌️
Adds a
command-not-foundhook for Nushell.Closes #264.