-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat: new rule for nix-shell
#1393
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing!
Please consider my comment below.
thefuck/rules/nix_shell.py
Outdated
bin = command.script_parts[0] | ||
return ( | ||
"nix-shell" not in command.script | ||
and "command not found" in command.output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your example:
$ ponysay moo
The program 'ponysay' is not in your PATH. You can make it available in an
ephemeral shell by typing:
nix-shell -p ponysay
$ fuck
nix-shell -p ponysay --run "ponysay moo" [enter/↑/↓/ctrl+c]
command not found
is not part of the output. Didn't you mean:
and "command not found" in command.output | |
and "command not found" not in command.output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your example, command not found is not part of the output
For commands that could be made available through nix:
- Indeed, the visible output in the terminal does not contain the text "command not found".
$ spt
The program 'spt' is not in your PATH. You can make it available in an
ephemeral shell by typing:
nix-shell -p spotify-tui
- But when we log the value of
command
to file, we do see "command not found":
Command(script=spt, output=/nix/store/p6dlr3skfhxpyphipg2bqnj52999banh-bash-5.2-p15/bin/sh: line 1: spt: command not found)
I'm not sure exactly why they're different, but it works 🤷🏼♂️.
Admittedly, it could be simpler if we could do:
- and "command not found" in command.output
+ and command.exitcode is 127
But I couldn't find a way to access the numeric exit code (127) from within the match
function. Only the human error message (command not found) is available it seems.
Finally, I've annotated the match conditions to make it clear what my intentions are for each.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes. That's because the command is executed through the shell — /bin/sh
. Would you please run thefuck in debug mode and look for the line that says DEBUG: Received output:
? e.g.:
THEFUCK_DEBUG=true thefuck spt
Among the debug messages there will be one with the output generated by the shell. Could you then please update the output values in mocked_nixpkgs
?
Also, how about writing tests for match
? 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests for match
done
I've cleaned up the PR a fair bit. As for the mock output, I've indicated which command I've used to get them so their purpose should be more clear now. If you notice that I've missed anything let me know. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware we could return more than one new command. Instead of just returning one, I've updated the rule to return them all. For example, the lsof
program is provided by two packages*:
busybox
: which bundles a bunch of common UNIX utiliteslsof
: standalone
* If you follow the link, you can click "Show more package details" to view the programs provided by each package.
Been loving this. Hope it gets reviewed and upstreamed for all. Thanks for making this! |
I've just discovered This PR would fit like a glove for me that just switched to nixos and haven't yet grown the muscle memory of typing the |
i would love to see this merged as well |
looks like you can use this already using e.g. an overlay, altho i had a bit of trouble getting it to work out of the box.
i had tried removing the added test, altho that appeared not to resolve the issue. |
it would seem cool to similarly get an approach using edit: KiaraGrouwstra@81d6786 |
I've been using a custom rule that supports the new unified CLI for a while, and was planning on opening a PR once this one has been merged (I hesitate to update this current PR as it's already tested and ready to be merged). I don't know if that will happen soon, so in the meantime I've pushed the changes to this new branch instead, which builds on this here PR. You can use the updated rule by adding it as a custom rule to your config. In the new rule, three variants are suggested. Assuming I run
Thoughts on future updates:
|
@thenbe i agree integrating with |
just tried these with a command like |
I'm not sure if If I make a typo |
@thenbe hm, i'm not sure.
feels like it knows about the whole command given it's reproducing it? |
another common nix thing we might be able to address from edit: KiaraGrouwstra@16d838b |
@thenbe what was the argument to favor |
If I'm only looking to execute a program (and don't need to be dropped into a shell) then I prefer I also recall
I've added this variant (the 4th one in my previous post), but disabled it after a while when I realized that I never reach for it. Do you find that you still need it over |
This would be useful. Does it still complain about the |
i've been using what was the by the way, had you managed to also package your branch for nix? considering i seemed to need that |
I just have it aliased to I opted not to package it for nix separately since # home.nix
home.file.".config/thefuck/rules/nix-shell.py".source = config.lib.file.mkOutOfStoreSymlink "${config.home.homeDirectory}/mydotfiles/thefuck/rules/nix-shell.py"; This way I don't need to rebuild every time I tweak the rule.
The unified CLI commands ( output
# it wants this instead:
$ NIXPKGS_ALLOW_UNFREE=1 nix shell nixpkgs#github-copilot-cli --impure |
aah i see! i'd yet to take that into account. 🙈 specifying the rules rather than doing overlays makes sense - thanks! |
Implementation is similar to the one explained in #912 (comment).
In a nutshell, it tries to wrap the user's failed command in a nix-shell call.
Further info on nix-shell: https://thiagowfx.github.io/2022/02/nix-shell-in-a-nutshell/#hello-world-classic