Skip to content
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

Merged
merged 5 commits into from
May 13, 2023

Conversation

akinomyoga
Copy link
Collaborator

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.

@akinomyoga akinomyoga force-pushed the _comp_compgen-4 branch 2 times, most recently from 670eb47 to fc2a09c Compare May 9, 2023 08:07
Copy link
Owner

@scop scop left a 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.

bash_completion Outdated Show resolved Hide resolved
bash_completion Outdated Show resolved Hide resolved
bash_completion Show resolved Hide resolved
bash_completion Show resolved Hide resolved
bash_completion Outdated Show resolved Hide resolved
bash_completion Outdated Show resolved Hide resolved
completions/_modules Outdated Show resolved Hide resolved
completions/bts Outdated Show resolved Hide resolved
bash_completion Show resolved Hide resolved
@akinomyoga akinomyoga force-pushed the _comp_compgen-4 branch 3 times, most recently from 9757d05 to ad8e810 Compare May 11, 2023 08:01
Co-authored-by: Ville Skyttä <ville.skytta@iki.fi>
@akinomyoga akinomyoga force-pushed the _comp_compgen-4 branch 2 times, most recently from 661290e to e7231c8 Compare May 11, 2023 08:35
@@ -42,7 +42,9 @@ _comp_cmd_cppcheck()
return
;;
-j)
COMPREPLY=($(compgen -W "{2..$(_ncpus)}" -- "$cur"))
local ret
Copy link
Owner

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.

Copy link
Collaborator Author

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.

Copy link
Owner

@scop scop left a 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 👍

@akinomyoga akinomyoga force-pushed the _comp_compgen-4 branch 3 times, most recently from a44301c to 53e2519 Compare May 11, 2023 23:23
Co-authored-by: Ville Skyttä <ville.skytta@iki.fi>
@akinomyoga akinomyoga merged commit a7fa31b into scop:master May 13, 2023
@akinomyoga akinomyoga deleted the _comp_compgen-4 branch May 13, 2023 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants