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

refactor: API/naming of other functions in bash_completion #1035

Merged
merged 11 commits into from
Sep 17, 2023

Conversation

akinomyoga
Copy link
Collaborator

@akinomyoga akinomyoga commented Aug 13, 2023

Draft PR to wait for #1028 and #1034

@akinomyoga akinomyoga force-pushed the refactor-api-4 branch 4 times, most recently from 7f41408 to d26fc05 Compare August 13, 2023 02:59
@akinomyoga akinomyoga force-pushed the refactor-api-4 branch 2 times, most recently from 8f3e8d6 to c99cf73 Compare August 26, 2023 12:25
@akinomyoga akinomyoga changed the title [Waiting for #1028 and #1034] refactor: API/naming of other functions in bash_completion refactor: API/naming of other functions in bash_completion Aug 26, 2023
@akinomyoga akinomyoga marked this pull request as ready for review September 11, 2023 06:18
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, pre-approved with some minor comments, whether addressed/not.

completions/slapt-get Outdated Show resolved Hide resolved
completions/slapt-src Outdated Show resolved Hide resolved
Comment on lines +2106 to +2107
local _shell _rest
while read -r _shell _rest; do
Copy link
Owner

Choose a reason for hiding this comment

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

Not actually introduced by this PR in any way, but thought I'd mention it: the underscore variable _ is used to mark throwaway ones in various languages, e.g. Python, Go, and Rust. I think it works with bash, too, but I feel it's not really meant for that, even though it's somewhat similar. Are you aware of any drawbacks of doing that or have thoughts about it?

Making use of it would reduce this to

Suggested change
local _shell _rest
while read -r _shell _rest; do
local _shell
while read -r _shell _; do

If we decide to make use of this, could do in another PR, no need to do it here.

Copy link
Collaborator Author

@akinomyoga akinomyoga Sep 17, 2023

Choose a reason for hiding this comment

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

I think it is technically possible but hesitate to promote the usage. $_ already has a meaning in Bash. It is a Bash special parameter, which represents the last word of the previous command. Bash allows operations that try to modify the value of _, but those operations do not take any effects (edit: it actually has effects but will be overwritten immediately), i.e., the assigned values are discarded. This behavior is actually preferable for using it as a dummy variable (as in read -r a b _). [ Note: The value of _ is updated at the end of the execution of each command, so one always see '_' as the value of _ after calling read -r a b _. ] One concern is that this non-standard overloading of usage might confuse users who read the bash-completion codebase, but this may be worrying too much.

Copy link
Collaborator Author

@akinomyoga akinomyoga Sep 17, 2023

Choose a reason for hiding this comment

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

OK, there are actually existing uses of _ in the codebase:

./completions/apt-get:54:                            while IFS=' |' read -r _ version _; do
./completions/mr:14:        printf %s "$help" | while read -r _ options cmd _; do

The other way of using rest is also found:

./completions/secret-tool:21:            while read -r first second third rest; do
./completions/strace:40:                            while read -r define syscall rest; do
./completions/strace:52:                                while read -r define syscall rest; do

Variable names are given in the following cases, but the last ones are not actually referenced in the later parts:

./completions/_umount.linux:52:    while read -r fs_spec fs_file fs_other; do
./completions/feh:47:            while read -r theme_name theme_opts; do
./completions/mplayer:259:                while read -r i j k; do

Copy link
Owner

Choose a reason for hiding this comment

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

I've used it here and there (not only in this repo, not sure if I introduced those mentioned or not) and don't ever remember coming across a problem with it. But sure, not (ab)using it for this is the safer way. Anyway, as said, no need to take care of it in this PR, and I wouldn't object to a PR that removes those usages in favour of the conventional name. Not sure what that would be for others besides the remainder though (rest works well for that).

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 searched for uses of read _ in Google (bash read "_" - Google Search) and found that this usage is actually used by many people.

But I also found that Greg's Wiki doesn't seem to recommend it while it says that it's a common Bash convention. The reasoning seems to be the portability to the other shells.

Some people use the throwaway variable _ as a "junk variable" to ignore fields. It (or indeed any variable) can also be used more than once in a single read command, if we don't care what goes into it:

# Bash
read -r _ _ first middle last _ <<< "$record"

# We skip the first two fields, then read the next three.
# Remember, the final _ can absorb any number of fields.
# It doesn't need to be repeated there.

Note that this usage of _ is only guaranteed to work in Bash. Many other shells use _ for other purposes that will at best cause this to not have the desired effect, and can break the script entirely. It is better to choose a unique variable that isn't used elsewhere in the script, even though _ is a common Bash convention.

@scop scop merged commit 4605edf into scop:master Sep 17, 2023
7 checks passed
@akinomyoga akinomyoga deleted the refactor-api-4 branch September 17, 2023 17:42
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