-
Notifications
You must be signed in to change notification settings - Fork 21
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: add bash completion #123
base: main
Are you sure you want to change the base?
Conversation
exe/Main.hs
Outdated
withJSON x = "-v" : "--log-format" : "internal-json" : x | ||
withJSON x = x <> ["-v", "--log-format", "internal-json"] |
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.
What is the purpose of this change?
From my understanding this means, that the user can’t override the debug-level anymore.
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.
The purpose is to make the NIX_GET_COMPLETIONS number correct. It is a number indicating which word of the input is being completed (eg: tabbing on a word in the middle of a command). Thus means we can use the Nix completion logic as-is.
Another approach could be to adjust that logic in the completion code with a +3 to account for the extra args.
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.
The completion code has no clue how we will be calling Nix, right? I can’t imagine a completion that would behave differently because of those two extra arguments. I vote for not changing the Haskell, but leaving the completion like it is. That seems good enough.
The bigger problem is, that this will complete e.g. nom copy
although nom does not support that.
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.
The way the Nix completions works is that we pass in the argument number of the thing being completed. So for example, you can run this directly to experiment...
[tom@tframe:~]$ NIX_GET_COMPLETIONS=2 nix build nixpkgs#hello
attrs
nixpkgs#hello
nixpkgs#hello-unfree
nixpkgs#hello-wayland
and it works if you have the cursor in the middle of the line; notice the "2".
[tom@tframe:~]$ NIX_GET_COMPLETIONS=2 nix build nixpkgs#hello --json -L
attrs
nixpkgs#hello
nixpkgs#hello-unfree
nixpkgs#hello-wayland
Because nom injects three extra arguments, the end result would be:
[tom@tframe:~]$ NIX_GET_COMPLETIONS=2 nix build -v --log-format internal-json nixpkgs#hello<TAB>
Which would complete the "-v" argument, not the expected one. The bash completer logic in _complete_nix does this. The alternative I mentioned (and was my original implementation) is to copy the nix completion logic, make a specific one for nom and modify it to adjust for the extra arguments:
https://github.com/NixOS/nix/blob/f0180487a0e4c0091b46cb1469c44144f5400240/misc/bash/completion.sh#L18
- done < <(NIX_GET_COMPLETIONS=$cword "${words[@]}" 2>/dev/null)
+ done < <(NIX_GET_COMPLETIONS=$(( cword + 3 )) "${words[@]}" 2>/dev/null)
Lastly, you can write your own completion handler, and call nix correctly yourself and write a completions/completion.bash to use that. This gives you the most control, but less re-use.
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.
Yeah, but what I am saying is that the completion engine has no clue that we will be calling nix build
with -v --log-format internal-json
. When you type nom build nixpkgs#hel<TAB>
it will use the completion for nix build nixpkgs#hel
and then after the user selected nixpkgs#hello and pressed enter we can insert flags wherever we want. That can’t mess with the completion.
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! Good point.Then I don't know why I I had to add this. Perhaps left over from the previous style of implementation. I'll remove.
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.
No, now i need to back on that statement. The original exec nom
is called by _complete_nix bash function in "${words[@]}
, so we do need a correction somewhere. I updated it to another approach where nom
detects this case can calls the underlying nix without the extra args.
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.
Oooh, the completion function is calling nom. Now I understand.
@@ -0,0 +1,2 @@ | |||
__load_completion nix | |||
complete -F _complete_nix nom |
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.
Who would have thought that it’d be that simple.
Thank you very much. This is a long-awaited improvement! I left a few comments. |
Have you tested this successfully? When I test this I get the output below, where I think the noise with the "Finished at" will probably break completion?
|
I also was unable to get this to work at all, even with futzing to remove |
Note: this is just for the
nom <subcommand>
usage. But a similar trick can be used fornom-build
.Adds bash completion calling nix's and correcting the order of the args.
Another approach would be to call the normal bash nix completion but correct for the additional three arguments injected by nom, but this seems a bit cleaner to chain load the completion script.
Fixes: #31