Skip to content

refactor: rename completion functions #1037

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

Merged
merged 7 commits into from
Nov 27, 2023

Conversation

akinomyoga
Copy link
Collaborator

This is a suggestion for the naming of completion functions (i.e., the functions that are supposed to be specified to complete -F) defined in bash_completion: In this PR, the completion functions in bash_completion have the names of the form _comp_complete_*.

@akinomyoga akinomyoga changed the title [Waiting for #1028] refactor: rename completion functions refactor: rename completion functions Aug 26, 2023
@akinomyoga akinomyoga marked this pull request as ready for review September 10, 2023 11:00
@scop
Copy link
Owner

scop commented Sep 12, 2023

Not sure about this one; it's certainly clearer this way, but if we go with this, it kind of asks renaming of _comp_cmd_* similarly as well -- they're intended for compgen -F, too. But that would then take them further from their _comp_cmd_*__* utilities, and those should not be renamed according to this plan.

@akinomyoga
Copy link
Collaborator Author

akinomyoga commented Sep 13, 2023

Hmm, that is a good point. Do you have any other ideas? The completion functions (which call _comp_initialize, etc. at the top) clearly form a class of the functions, but that classification is not so clear from the function names in the current form. I think that having a particular form of names for this function class makes it easier to find and choose the correct function.

For consistency, the current way is already not so consistent; while the completion function for a specific command is named as _comp_cmd_${CmdName}, there doesn't seem any naming rules for the general completion functions defined in the main file bash_completion. Besides, looking at the section of Naming in api-and-naming.md, I think there is already a separation of the naming rule between the functions in bash_completion and those in completions/*. Possible solutions would be

  1. To adopt this PR. As pointed out, this seems to introduce some inconsistency in the naming rule of the completion functions in the main file and specific completion functions.
  2. One way is to give up giving them a particular function name and to keep the completion functions in bash_completion at the top level directly under _comp_*.
  3. The other way around might be to rename all the other functions (which are not completion functions) to _comp_util_* (probably except for _comp_compgen_*). I've checked the list of functions in bash_completion, but it doesn't seem right to rename everything including _comp_initialize to _comp_util_initialize, _comp_load to _comp_util_load.
  4. Or to rename specific-command completion functions as _comp_cmd_${CmdName}_complete or _comp_cmd_${CmdName}_cmoplete_${CmdName}. This seems redundant.
  5. (Others?)

Currently, I feel options 1 and 2 would be realistic. Considering the fact that we already have a separation in the naming rule of functions in the main file bash_completion and specific completions completions/*, option 1 appears acceptable to me.

In any case, I should probably update doc/api-and-naming.md as well.

| private non-local mutable variables | `_comp__*_mut_*` | `_comp_cmd_${Command}__mut_${Data}` |
| exporter function local variables | `_*` (not `_comp*`) | `_*` (not `_comp*`) |
| public/exported functions | `_comp_*` | `_comp_xfunc_${Command}_${Utility}` (functions for use with `_comp_xfunc`) |
| - completors (for `complete -F`) | `_comp_complete_*` | `_comp_cmd_${Command}` |
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this, _comp_complete_load is in an incorrect namespace as it does something else, we should look into renaming it to something else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my thinking, _comp_complete_load is still in the same class of "completion functions" in the sense that _comp_complete_load is intended to be specified to complete -F.

Actually, when I introduced the name _comp_complete_load in d9082d2 (#1032), I already anticipated the changes of this PR and made consistent with this PR. This is also one of the reasons that I split _completion_loader into two functions with different purposes, _comp_load (for a utility to purely load a completion) and _comp_complete_load (for complete -F) (as in comment d9082d2#r1295226734).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, indeed. No problem then, merging. Thanks!

@akinomyoga
Copy link
Collaborator Author

akinomyoga commented Nov 22, 2023

squashed typo fix complet{or => er}

@scop
Copy link
Owner

scop commented Nov 27, 2023

For some reason GH is stalling in "Checking for ability to merge automatically…" for me, feel free to merge at your convenience if it works for you.

@akinomyoga
Copy link
Collaborator Author

Thank you!

@akinomyoga akinomyoga merged commit 15f3ecc into scop:master Nov 27, 2023
@akinomyoga akinomyoga deleted the refactor-api-6 branch November 27, 2023 20:59
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