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

refactor: rewrite command-not-found.sh in rust #227

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

figsoda
Copy link
Member

@figsoda figsoda commented Jun 17, 2023

closes #222
closes #126
should also fix #210 once this is finished

the implementation is adapted from locate and command-not-found.sh

draft because there are a few things I need to do:

  • actually replace the command-not-found.sh, including fish support
  • complete has_flakes to read user config, also consider keeping the previous manifest.json check
  • better error handling, currently some errors are silently ignored
  • make the code a bit more compact, the Command::news feel a bit verbose

@figsoda figsoda marked this pull request as draft June 17, 2023 01:43
@emilazy
Copy link

emilazy commented Jun 17, 2023

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...)

@figsoda
Copy link
Member Author

figsoda commented Jun 17, 2023

I just found a typo in the auto run error messages that's existed for a few years now

Failed to install $toplevel.attrs.

Failed to install $toplevel.attrs.

that's probably a sign that people are not really using this feature

@emilazy
Copy link

emilazy commented Jun 17, 2023

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. bad-command | ...). Not sure if there's a better check that could be done on the fish side and passed in via environment or something. Maybe it's fine to just check stderr and output there though? Seems okay for a piped command to print to stderr and then fail.

@figsoda
Copy link
Member Author

figsoda commented Jun 17, 2023

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?

@emilazy
Copy link

emilazy commented Jun 17, 2023

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 2>&1 or similar.

(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...)

@bennofs
Copy link
Collaborator

bennofs commented Jun 17, 2023

I agree that NIX_AUTO_RUN / NIX_AUTO_INSTALL are probably fringe usecases that we can drop from upstream (people who want this behavior can use comma / use their own command-not-found handler).

We should include a message if NIX_AUTO_RUN / NIX_AUTO_INSTALL is set that explains the alternatives (comma, custom command not found) so users are not surprised that the feature is suddenly no longer available.

Flake lock file updates:

• Updated input 'nixpkgs':
    'github:NixOS/nixpkgs/75a5ebf473cd60148ba9aec0d219f72e5cf52519' (2023-06-11)
  → 'github:NixOS/nixpkgs/e603dc5f061ca1d8a19b3ede6a8cf9c9fcba6cdc' (2023-06-22)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants