Skip to content

refactor: API/naming of functions and a variable in bash_completion #1032

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
Sep 7, 2023

Conversation

akinomyoga
Copy link
Collaborator

@akinomyoga akinomyoga commented Aug 11, 2023

The remaining functions in bash_completion are going to be processed in other PRs:

@akinomyoga akinomyoga force-pushed the refactor-api-1 branch 2 times, most recently from 37c93f9 to 9c0a164 Compare August 13, 2023 00:36
@akinomyoga akinomyoga changed the title Rename and refactor several functions and a variable in bash_completion refactor: API/naming of functions and a variable in bash_completion Aug 13, 2023
@akinomyoga
Copy link
Collaborator Author

commit e633b62 ({ => _comp_compgen}_shells) was moved to PR #1035 1f8e77c.

# TODO:API: rename per conventions
__load_completion()
# @since 2.12
_comp_load()
Copy link
Owner

Choose a reason for hiding this comment

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

I'm wondering if this is something we want/need to expose as a public function? External completions shouldn't need to use it I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you.

Hmm, ble.sh actually wants to use it. _completion_loader (which is also used by git-completion.bash from the Git project) sets _minimal up when none is found, but ble.sh wants to fall back into its own completion if none is found. Another option is to make _completion_loader accept an option that suppresses the fallback to _minimal.


I now noticed that I overlooked _completion_loader in renaming functions. I think _completion_loader should be renamed to _comp_load instead of __load_completion. I'll fix it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another option is to make _completion_loader accept an option that suppresses the fallback to _minimal.

I suggested this, but _completion_loader is used as a completion function for complete -F, so I realized it causes a problem when a user inputs a command name starting with - in the command line. So we should prepare two separate functions for complete -F and for a feature to load the completion from a script.

Also, as far as I search on GitHub, __load_completion is directly used by some users (regardless of whether it was originally intended by us).


The function _completion_loader currently seems to be intended for two different roles. One is as a function that is specified to complete -F. For example, returning status 124 on successful loading is a part of this use case. The other is as a function that can be called by a user to load a completion for a specific command. Here, I suggest splitting _completion_loader into two: 1) _comp_xxx "$cmd" that can be used to load a completion for the specified command, and 2) _comp_yyy that can be specified to complete -F.

Then, I would also like to have an option that does not define the fallback completion _minimal when the specific completion is not found. I use it for ble.sh. There are also other scripts that use __load_completion (though I'm not sure if they recognize _completion_loader). There are two ways:

  • Opt-out _minimal -- _comp_xxx -- "$cmd" defines the fallback setting _minimal by default, and _comp_xxx -R "$cmd" does not.
  • Opt-in _minimal -- _comp_xxx -- "$cmd" does not define _minimal, and _comp_xxx -D "$cmd" defines _minimal.

The opt-in behavior seems natural to me as defining _minimal is an additional behavior on top of just loading a specific completion. Actually, the opt-in behavior is mostly the same as the current implementation of __load_completion, so my current suggestion is to merge the feature _comp_xxx (feature 1 of _completion_loader) into __load_completion as an option -D (defining the default setting _minimal) and to make it a public interface under the name _comp_load.

The feature _comp_yyy is named as a function _comp_complete_load in accordance with the naming convention suggested in #1037. I can adjust the naming.

I've pushed those adjustments in commit 77497bc.

@akinomyoga akinomyoga force-pushed the refactor-api-1 branch 3 times, most recently from 77497bc to 9ef0024 Compare August 15, 2023 23:17
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, thanks!

@scop scop merged commit 84921b8 into scop:master Sep 7, 2023
@akinomyoga akinomyoga deleted the refactor-api-1 branch September 9, 2023 10:18
@akinomyoga
Copy link
Collaborator Author

Thank you.

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