Skip to content

refactor(bash_completion): rename functions _userland, _sysdirs, _have, and _rl_enabled #735

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
May 24, 2022
Merged

refactor(bash_completion): rename functions _userland, _sysdirs, _have, and _rl_enabled #735

merged 5 commits into from
May 24, 2022

Conversation

akinomyoga
Copy link
Collaborator

  • This is still a draft; only a small part of the functions are processed.
  • This is based on feat: add _comp_xfunc #734 (_comp_xfunc)
  • I open this PR to reference the related PRs because some of my existing branches already contain some part of refactoring names and behaviors, so I'd like to process them at the same time.

@akinomyoga akinomyoga mentioned this pull request Apr 11, 2022
Merged
@akinomyoga akinomyoga force-pushed the refactor-function-names branch from 4bc7856 to 5f5a32c Compare April 17, 2022 21:32
scop
scop requested changes Apr 23, 2022
View reviewed changes
@scop
Copy link
Owner

scop commented Apr 23, 2022

This is still a draft; only a small part of the functions are processed.

I wouldn't worry about that -- we can process things in small chunks, that way they are easier to review.

@akinomyoga
Copy link
Collaborator Author

akinomyoga commented Apr 23, 2022 •
edited
Loading

Thank you for the review!

I wouldn't worry about that -- we can process things in small chunks, that way they are easier to review.

Now, I agree with it because I seem to have many things to discuss in the refactoring. I think I'd like to separate the PRs. I will change the PR subject.

Maybe I could have just changed the function names by blindly prefixing _comp_, but I feel it would be better to reconsider the interface (print to stdout vs assigning to variables) and the naming of the functions rather than introducing another breaking change later. But, if you think we can just prefix _comp_ to the existing function names keeping the interface for the fast release of 2.11 2.12, I can follow the suggestion.

@akinomyoga akinomyoga force-pushed the refactor-function-names branch from c080a00 to d14308a Compare April 23, 2022 12:18
@akinomyoga akinomyoga changed the title refactor function names in bash_completion refactor(bash_completion): rename functions _userland, _sysdirs, _have, and _rl_enabled Apr 23, 2022
@akinomyoga
Copy link
Collaborator Author

akinomyoga commented Apr 23, 2022 •
edited
Loading

I have rebased it. I have decided to narrow the scope of this PR to these 5 functions and update the PR subject. Now this is ready for review.

Another thing related to the function naming is about _comp_has_command (previously _have). If the original function name shouldn't be changed too much or there are any better suggestions, I can adjust the name.

Edit: Also, if you want to do any changes/renaming for other functions in bash_completion, please feel free to do that without notice.

@akinomyoga akinomyoga marked this pull request as ready for review April 23, 2022 12:23
scop
scop approved these changes May 5, 2022
View reviewed changes
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 a few comments, feel free to address the way you prefer or not, pre-approved either way.

bash_completion Outdated
# This function checks whether we have a given program on the system.
#
_have()
_comp_has_command()
Copy link
Owner

Choose a reason for hiding this comment

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

I would personally go with _comp_have_command for this -- the "has" makes it read like asking if completion has a command which is not what we're "asking" with this. _comp_have_command sounds just less odd to me. But not a biggie/blocker, leaving up to you.

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 for the comment. I have switched to _comp_have_command

bash_completion Outdated
}

# This function checks whether a given readline variable
# is `on'.
#
_rl_enabled()
_comp_rl_enabled()
Copy link
Owner

Choose a reason for hiding this comment

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

I was contemplating _comp_is_rl_enabled for this, but I think it's better as _comp_rl_enabled.

Then again there's little reason to keep it this short, perhaps _comp_readline_variable_on or _comp_readline_variable_enabled (or "variable" shortened to "var" in either) would be better.

Again, leaving up to you to decide.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I adopted _comp_readline_variable_on.

akinomyoga and others added 2 commits May 8, 2022 19:43
@akinomyoga @scop
refactor: { => _comp}_userland
c086fba 
Co-authored-by: Ville Skyttä <ville.skytta@iki.fi>
@akinomyoga @scop
refactor: { => _comp}_sysvdirs
6d6f07f 
Co-authored-by: Ville Skyttä <ville.skytta@iki.fi>
@akinomyoga akinomyoga force-pushed the refactor-function-names branch 3 times, most recently from edf207f to cbc0211 Compare May 12, 2022 05:27
@akinomyoga akinomyoga force-pushed the refactor-function-names branch from cbc0211 to fffc188 Compare May 12, 2022 05:34
@akinomyoga
Copy link
Collaborator Author

akinomyoga commented May 17, 2022 •
edited
Loading

Left a few comments, feel free to address the way you prefer or not, pre-approved either way.

Maybe does pre-approved mean that I'm supposed to merge it when I decided on the points raised in these comments?

@scop
Copy link
Owner

scop commented May 24, 2022

Maybe does pre-approved mean that I'm supposed to merge it when I decided on the points raised in these comments?

Yes, that's what I meant, apologies for being unclear (and again taking such a long time to get back to this!). I'll go ahead and merge now, thanks!

@scop scop merged commit 2dfc8ad into scop:master May 24, 2022
@akinomyoga
Copy link
Collaborator Author

Thank you! I'll keep it in mind from now on!

@akinomyoga akinomyoga deleted the refactor-function-names branch May 24, 2022 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scop scop scop approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@akinomyoga @scop