-
-
Notifications
You must be signed in to change notification settings - Fork 423
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
programs.nix-index: add common-not-found support for Fish #272
Conversation
I don't know much about fish, but I think the fish integration uses foreign-env/babelfish to interact with standard shell scripts. Might also be relevant here, would be great of somebody that actually uses fish could take a look. |
So, as far as I can tell, With #270 now merged, it should be possible to add something like the following to the programs.fish.interactiveShellInit = mkIf config.programs.fish.useBabelfish ''
function __fish_command_not_found_handler --on-event="fish_command_not_found"
command_not_found_handle $argv
end
''; Unfortunately, it appears that currently Maybe others like @kevingriffin, @bouk, or @toonn, might have ideas on the best way to move this forward? |
I think the simplest way forward is to petition the project to include a fish version of the handler too, shelling out to bash probably introduces a noticeable delay. I'd also prefer if this was easy to disable. I don't personally use such hooks. |
There's currently and issue about that on the In this case, It doesn't look like shelling out to Bash is having any noticeable effect. Here are the results of a quick benchmark I just ran:
So, the only thing |
As some additional context, in the benchmark in my previous comment, most of the execution time is actually time it takes to show the output. Removing the
So, at least on my system, shelling out to Bash, sourcing, then running the function adds about 3ms on average. |
Note that my performance remark is inconsequential if it's barely noticeable anyway. If this is the only reason to enable the module I agree a flag's not necessary. |
Totally.
Sorry, I think I might not have been very clear about what I'm comparing with the benchmark, or at least trying to compare. I claim (though it's possible of course that I'm confused/wrong) that what I'm comparing with that benchmark, is the difference between,
I think this is the relevant comparison since, in the absence of there being a native Fish implementation (or using Babelfish to create one from the current version), the only way for Fish to run the command is the run it inside a sub-shell that supports the current implementation (the workaround I implemented here). It seems to me like the key question when trying to figure out whether this workaround incurs a significant performance penalty, is to look at how much longer it takes to spawn a new bash shell, source the script, and run the command, compared to just running the command directly. To do that, I ran
Just to clarify. It's my understanding that the only difference between the two versions of the benchmarks I posted is whether the output of the commands is printed to the terminal or not. Both benchmarks are running the command. So it's actually the case that running the command takes a negligible amount of time, and the bulk of the time is taken up waiting for the terminal to render the output (I'm using Kitty). (Edit: Actually, it's not the terminal rendering, I ran the first benchmark again piping the output of both commands to |
With #282 now resolved with the latest version of Babelfish, I've updated the PR accordingly, so that it uses the Fish native command that Babelfish creates, if |
Gonna close this since it's been open for a while now, and I don't care very much about it. |
Works like a charm for me!