systemd
Shell completion
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.
This is pretty nice. If only we could make zsh work too :)
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 .write calls instead of print, since I find it nicer for the crufty strings this is building.
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... :)
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 :)
Ooh, nice, under zsh, completions for -d and -t work perfectly.
The same under bash.
So I think this is functionally complete.
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 less. The output from mkosi seems to be just pure ASCII. Maybe zsh tries to do a read at exactly the wrong time and this causes … something?
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 CompGen enum, since it lives in Args, needs to stay in config, though, so as to not have a cyclic import.
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.
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.
Also, adjust the formatting in the bash resource to follow the usual style with 'if something; then' on one line.
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 CompGen usable can happen later, that is why the annotation for them was in config. Daan and I spoke about trying to look up the types on the Config object, but we don't need to wait for this, the current form should be good enough as a start and stylistic quibbles can be changed later.
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.
It is green, except for bogus comments from shellcheck that cannot be silenced.
Oh, OK, I guess this requires a longer explanation: shellcheck could be silenced by inserting suppression comments into the scripts. But we very do not want to do this, because that would mean that we cannot check the whole script. I added a test to check the whole script, and that passes without any warnings.
Sadly, differential shellcheck is not flexible enough here.
@keszybz The differential-shellcheck github action has an exclude-path option. We can use that.
I added a commit with a forward definition of the (hash) arrays, that should hopefully appease the differential shell check
The differential-shellcheck github action has an
exclude-pathoption. We can use that.
I looked at docs, but I didn't see it. That would work.
I added a commit with a forward definition of the (hash) arrays, that should hopefully appease the differential shell check
That also works…