-
Notifications
You must be signed in to change notification settings - Fork 304
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
Shell completion #2779
Shell completion #2779
Conversation
bash completion would be really nice, yes please |
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.
This is pretty nice. If only we could make zsh
work too :)
Some issues I notice in quick testing:
- we only allow one verb, so if there's a verby-looking word already present, we shouldn't propose verbs.
- most verbs don't accept options after the verb. Thus,
mkosi build -
should not complete our options. Either nothing or a general command completion like forsudo
.
I have a few long train rides upcoming this week and I've been reading the rather dense zsh completion documentation ;) |
Pushed an updated version, making this just an command line option instead of a verb, this also sidesteps the shell autodetection. This version adds zsh, but I've still not tested zsh nor fish, since I forgot to install them before boarding my train. I've kept the I've not yet thought about not proposing verbs multiple times, because in the case of the bash completion this will necessitate changing the approach for the completion completely, since the whole argument string needs to be scanned and I think this can't use the "stateless" approach of looking at the current and preceding option. No idea how fish and zsh handle this, but generally all three need a few more completion generations. I think the shell check things are mostly gunk, except maybe the thing about mapfile, this needs looking into. Please, nobody ask for another shell, bash and zsh are scary enough, I do hope nobody is still using csh or ksh... :) |
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 hammered the zsh part until is started to behave.
Another train ride another battle with shell completion. I tested the generated completions for bash, fish and zsh and at least they don't throw errors on me and complete the options I tried. The zsh version now has working choices, too, which didn't work for me during testing and the zsh version has the constant stuff factored out into a resource, like the bash version. Adding more completers might necessitate adding a resource for fish as well. There's still a lot of room for improvement (boolean options that can also take various things that we parse as booleans, options that take comma- or otherwise separated lists), but the basic stuff is there. The three completions are as similar as possible and as idiomatic as I was able to make them without being a user of fish or zsh (thanks @keszybz!). The bash version, while idiomatic and using what bash provides (it's O(1)!), will happily complete multiple verbs. That will need fixing, but that's a rewrite of the resource to parse the whole command line on every completion. I think I'd like to leave that for later. Please give it a try to see what breaks :) |
d560b89
to
a6e6a02
Compare
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.
Can't we pass the shell for which to generate completions for as an argument and then make this a new verb completion
? I don't like making this an option tbh.
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.
if we made this a verb, we could also move all this logic into its own file completion.py
Ooh, nice, under zsh, completions for I don't really have an opinion on verb vs. option choice. Previously, I asked for the verb to be removed because it was had both an option and the verb. But having the verb without an option is also fine. In my local testing, I get this strange result: $ bin/mkosi --shell-completion=bash | less
...
declare -A _mkosi_compgen=([-C]=_mkosi_compgen_dirs [--directory]=_mkosi_compgen_dirs)
:[13] + 2689831 suspended (tty input) bin/mkosi --shell-completion=bash |
2689832 suspended (tty input) less It's stochastic, but happens ~80% of the time. I don't understand what is going on here, but I think it may be a bug in |
I have no strong feeling regarding whether this is an option or a verb, I'll change it back to a verb. |
Pushed a rework into a verb that also moves most of the stuff into a separate module. The |
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.
Can we fix the shellchecks as well? I didn't review the generation stuff too closely but it all looks reasonable, the comments are mostly about decoupling this a little more so it's self contained.
Still need to test the fish completion
I force-pushed with the changes that were requested. I want to get this merged… I thought it'd be a few small cleanups, but then it turns out to be more work. I hope that's OK. |
Thanks! That's why I didn't get around to it this week, a few deadlines to keep.
I gave you the style in zsh, give me bash. I strongly detest semicolons in shell scripts and the two line style is the way ALGOL intended it. ;) Joke aside, the style with semicolon is more common, but I do think the two-line style is more readable, makes visually nicer blocks and looks nicer when the predicate or what one wants to loop over takes more than one line, which happens easily. I'm not married to it, though, and the further rework to make the different |
I undid the style change to |
ConfigAction used to be a dynamically generated class, but fortunately it's not anymore, so we can simplify this.
First iteration for bash only.
This completion of verbs is based on _timedatectl in systemd repo. Completion for short options doesn't work. It also doesn't work for timedatectl, so this needs to be fixed in both places. Co-authored-by: Zbigniew Jędrzejewski-Szmek <[email protected]>
Follow-up for c036450.
Both changes requested in review. The output generator for bash,fish,zsh is unchanged. Also do minor whitespace and style adjustments as requested in review.
readarray is used to create arrays. The one clear advantage is that we don't need to override $IFS. Together with the change to not assign an unused variable, this removes shellcheck warnings. Nevertheless, shellcheck would still warn about the file because it doesn't know about the variables that are in the part that is generated dynamically. Also, move more content to the static resource file. The order of declarations doesn't matter, so it's fine if the variables are defined below the functions. Also, adjust the formatting in the bash resource to follow the usual style with 'if something; then' on one line.
Sadly, shellcheck does not support zsh [1], and it's not even possible to evaluate the script with zsh because it fails with: _arguments:comparguments:327: can only be called from completion function So the zsh script shall not be checked. [1] koalaman/shellcheck#809
OK, the tests pass and the comments have been addressed. Can we merge this and do follow-ups separately? |
shellcheck is still failing. I'm fine with disabling the checks but that should be fixed before merging. |
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.
LGTM but CI should be green
It is green, except for bogus comments from shellcheck that cannot be silenced. Oh, OK, I guess this requires a longer explanation: Sadly, differential shellcheck is not flexible enough here. |
@keszybz The differential-shellcheck github action has an |
I added a commit with a forward definition of the (hash) arrays, that should hopefully appease the differential shell check |
I looked at docs, but I didn't see it. That would work.
That also works… |
Dropped a shellcheck directive I accidentally committed. |
That's a rough draft I had lying around for a while now and polished up on the train. It needs more completers for actions and I haven't thought about whether the assumptions for the bash completion are sound and the fish completion is written looking just at the docs.
The idea is to be able to generate a completion script that distros can "build" during packaging and ship pre-built.