-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
refactor: rewrite command-not-found.sh in rust #227
base: master
Are you sure you want to change the base?
Conversation
Wonderful! The fish mechanism is simple but there are some nuances to consider that break the existing script (translated with babelfish); see fish-shell/fish-shell#9806, fish-shell/fish-shell#7902, fish-shell/fish-shell#9517. (tl;dr it's run inside a substitution so you can't check tty through stdout, which might also interfere with the auto-run feature in terms of what stdout they get and how the exit status is handled. I wonder if anyone actually uses auto-run though? comma seems like a less error-prone way of getting essentially the same result...) |
I just found a typo in the auto run error messages that's existed for a few years now nix-index/command-not-found.sh Line 56 in 73d96c8
nix-index/command-not-found.sh Line 69 in 73d96c8
that's probably a sign that people are not really using this feature |
Yeah, I would be happy to see it go. I think the main adaptation you'll need to make for fish is conditioning the pipe check on stdin or stderr, which unfortunately won't be as reliable (e.g. |
tbh I think it makes sense for this to run even if it is being piped into another command, maybe we can just remove the check altogether? @bennofs what do you think? |
Yeah I dunno exactly what the rationale is if no output is going to stdout anyway. I think checking stderr would be good because you're printing output there that could be misinterpreted if you're piping it into another command with (fwiw I think NIX_AUTO_INSTALL is even worse than NIX_AUTO_RUN – permanent imperative changes to system configuration in response to what could be a mere typo...) |
I agree that We should include a message if |
Flake lock file updates: • Updated input 'nixpkgs': 'github:NixOS/nixpkgs/75a5ebf473cd60148ba9aec0d219f72e5cf52519' (2023-06-11) → 'github:NixOS/nixpkgs/e603dc5f061ca1d8a19b3ede6a8cf9c9fcba6cdc' (2023-06-22)
This reverts commit 2030741.
Just leaving a note here that I'm building and running |
closes #222
closes #126
should also fix #210 once this is finished
the implementation is adapted from
locate
andcommand-not-found.sh
draft because there are a few things I need to do:
has_flakes
to read user config, also consider keeping the previousmanifest.json
checkCommand::new
s feel a bit verbose