style(bash-v2): various cleanups #1702
Conversation
bash_completionsV2.go
Outdated
| local subdir | ||
| subdir=$(printf "%%s" "${out}") | ||
| if [ -n "$subdir" ]; then | ||
| subdir=${out%%$'\n'} |
There was a problem hiding this comment.
Here, in the final bash script, do we want a single or a double %%? If we want a double one to remove all trailing new lines, we need to put four of them in go (%%%%)
There was a problem hiding this comment.
It doesn't make a difference; there are no globs involved, so we are removing just one character (the linefeed) no matter if we do % or %%. Given this, the former seems a bit cleaner to me here, which is why I chose that option.
There is indeed a small difference in behavior here: the former implementation removed all trailing linefeeds, this removes just one. But I assume we don't have cases where there would be more than one, I chose the simplest available approach enabled by that assumption.
One way to remove multiple with a simple parameter expansion would require use of extended globbing, which we currently do not rely on in the script. bash-completion is a requirement and it does enable extglob unconditionally, and I don't see that changing, so in that sense it would be safe to use it. In that case the expansion could be written as ${out%%%%+($'\n')} (in Go code).
Another a bit hacky way which doesn't require extended globbing would be to do Oops, no it wouldn't ;) Or hmm, I'm not actually sure, maybe it would, depends what ${out//$'\n'} which is semantically different, but would for practical purposes work just fine here I think.$out can have here. Too lazy to dig in deep to figure out right now :)
Let me know if you prefer one of these approaches and I'll adjust.
There was a problem hiding this comment.
It doesn't make a difference; there are no globs involved, so we are removing just one character (the linefeed) no matter if we do % or %%.
Ah yes. I missed that. Thanks.
I agree it should be safe to strip a single linefeed.
There was a problem hiding this comment.
Thanks once more @scop!
I'll just run a bit of extra manual testing later today, especially for the lesser used directive code paths and will merge after.
|
Just an update that I haven't forgot about this, I'm just having trouble with my setup (waiting on a new computer). I will get back to this as soon as I can. |
|
Sure, no rush at all as far as I'm concerned, take all the time you like 👍 |
|
@scop Finally getting back to this but the PR now has conflicts 😞 Could you rebase when you have a moment? |
scop
force-pushed
the
style/bash-v2-cleanups
branch
from
960ed90 to
b8b0703
Compare
|
Reworked and still untested, hopefully I didn't add too many bugs. |
Thanks @scop. After looking at this one again, I suggest changing our strategy. |
marckhouzam
added
area/shell-completion
The result of the expansion is null no matter if the variable is unset or null in both cases; the former form is arguably easier on the eye.
No longer needed as of f464d6c, saves a subshell.
Herestrings read cleaner than process substitutions, and work in posix mode (but we do and will have some process substitutions so this doesn't matter much). Both approaches may end up using temporary files.
scop
force-pushed
the
style/bash-v2-cleanups
branch
from
b8b0703 to
ef504a6
Compare
|
Rebased. |
|
I'm going to merge this improvement now to give it a lot of soak time until the next release. |
* use arithmetic evaluation in numeric context
* remove unnecessary $ from array index variables
* [[ ]] over [ ], == over =, remove unnecessary quoting
* use ${foo-} rather than ${foo:-} in emptiness check
The result of the expansion is null no matter if the variable is unset
or null in both cases; the former form is arguably easier on the eye.
* remove unnecessary trailing linefeed removal
Merge spf13/cobra#1702
Here's a bunch of bash code cleanups I made on the side while working on the performance improvements. See individual commits and messages for details. Some of these are also performance improvements in principle in addition to stylistic ones, but I believe they're not on any hot path so that side is negligible.
Some of these might be a bit opinionated/subjective but read the way I'd personally put them, let me know if you'd rather not make some of these changes and I'll follow suit.