-
Notifications
You must be signed in to change notification settings - Fork 392
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
Conversation
37c93f9
to
9c0a164
Compare
bash_completion
bash_completion
9c0a164
to
fa04a0c
Compare
# TODO:API: rename per conventions | ||
__load_completion() | ||
# @since 2.12 | ||
_comp_load() |
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.
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.
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.
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.
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.
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.
77497bc
to
9ef0024
Compare
Co-authored-by: Ville Skyttä <ville.skytta@iki.fi>
9ef0024
to
d9082d2
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.
LGTM, thanks!
Thank you. |
The remaining functions in
bash_completion
are going to be processed in other PRs:{ => _comp}{_get_first_arg,_count_args}
#1033_count_args
and_get_first_arg
which areWIP in another branch.akinomyoga:refactor-api-2
_comp_{get_first_word,count_args}
#1036akinomyoga:refactor-api-dev
{ => _comp_}root_command
plus misc small fixes #1034root_command
->_comp_root_command
bash_completion
#1035_known_hosts_real
, which needs to be updated to be a generator.__expand_tilde_by_ref
,_expand
,_variables
,_services
complete -F
. Maybe we can give consistent names to such functions, such as_comp_complete_*