-
Notifications
You must be signed in to change notification settings - Fork 388
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
fix: use _comp_compgen
with API changes
#973
Conversation
f823c9c
to
8c4c325
Compare
670eb47
to
fc2a09c
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.
Left some comments, not a through review yet, but these can already be looked into.
One conflict to resolve, too.
9757d05
to
ad8e810
Compare
Co-authored-by: Ville Skyttä <ville.skytta@iki.fi>
661290e
to
e7231c8
Compare
e7231c8
to
d5a78ce
Compare
@@ -42,7 +42,9 @@ _comp_cmd_cppcheck() | |||
return | |||
;; | |||
-j) | |||
COMPREPLY=($(compgen -W "{2..$(_ncpus)}" -- "$cur")) | |||
local ret |
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.
An unrelated note, but now that we're sprinkling local ret
quite a bit and more to come, perhaps adding it to the standard local cur prev words cword was_split comp_args
everywhere would not be a bad idea.
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 just my preference, but I'd like to keep the variable scope small generally. Maybe this results in possibly multiple declarations of ret
in a function, but I kind of accepted and got used to it because it doesn't cause a problem in Bash.
Including it in the standard variable list might be useful to prevent the variable leak in the case where one forgets to declare ret
in calling a function that sets ret
. But in another way of thinking, that hides the bug that one forgot to declare ret
. I cannot decide which one is better for this point.
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, good to merge once tests start to pass 👍
a44301c
to
53e2519
Compare
Co-authored-by: Ville Skyttä <ville.skytta@iki.fi>
53e2519
to
6b3dfa5
Compare
These are the cases where I modified interfaces of existing functions.
Some involve internal generators, but I haven't applied the possible suggested internal generator syntax
_comp_compgen -i CMD NAME
in #962 (comment). If the syntax would be finally employed, I'll update them in a future separate PR.