Skip to content

refactor(bash_completion): refactor { => _comp_}{dequote,quote} #736

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 11 commits into from
Jun 1, 2022

Conversation

akinomyoga
Copy link
Collaborator

In #735, I'd like to refactor the function names quote and dequote, but another branch that I created two months ago contains an improved version _comp_dequote and improved uses of quote, so I'd like to process this branch before #735 if possible.

@akinomyoga akinomyoga marked this pull request as draft April 11, 2022 22:51
@akinomyoga akinomyoga force-pushed the _comp_dequote branch 2 times, most recently from 9b4b037 to 74cf5f1 Compare April 12, 2022 10:55
@akinomyoga akinomyoga marked this pull request as ready for review April 12, 2022 11:44
@akinomyoga akinomyoga force-pushed the _comp_dequote branch 2 times, most recently from 8c3ae04 to 45233e8 Compare April 17, 2022 22:03
@akinomyoga
Copy link
Collaborator Author

Also, it should be noted that, in these commits, I have changed the interface of dequote and quote writing their results to stdout so that the new _comp_dequote and _comp_quote write the result in the variable ret. If you have any suggestions, please feel free to tell me that.

Another thing that needs to be discussed is the annotations like @param in the code documentation. I wanted to describe the variables that are supposed to be referenced/modified so used @var, but this might cause problems if these annotations are processed by some documentation programs that don't support @var. Are these annotations intended to be processed by a certain annotation language? I have used @var inspired by Doxygen, but I found by searching on the internet that the existing documentation seems to follow JavaDoc (which I'm not familiar with)?

@akinomyoga
Copy link
Collaborator Author

With other small fixes bf77c2f 3f49e1b, I have rebased/squashed the related fixup commits.

@akinomyoga akinomyoga changed the title reimplement dequote as _comp_dequote refactor(bash_completion): refactor { => _comp_}{dequote,quote} Apr 23, 2022
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! Left a few trivial comments, feel free to merge after addressing.

@akinomyoga
Copy link
Collaborator Author

shellcheck somehow considers the value of the local variable quoted (previously quote) is used in other functions that have the variables of the same name.

In bash_completion line 172:
    local quoted='\\.|'\''[^'\'']*'\''|\$?"([^\"$`!]|'$param'|\\.)*"|\$'\''([^\'\'']|\\.)*'\'''
                       ^-- SC2089: Quotes/backslashes will be treated literally. Use an array.


In bash_completion line 769:
        toks+=($(compgen "${opts[@]}" -- $quoted))
                                         ^-----^ SC2090: Quotes/backslashes in this variable will not be respected.


In bash_completion line 779:
            toks+=($(compgen -f ${plusdirs+"${plusdirs[@]}"} -- $quoted))
                                                                ^-----^ SC2090: Quotes/backslashes in this variable will not be respected.

I have decided to further change the variable names as { => regex_}{param,quoted} (ebf06de...c6bb062). Squashed and rebased.

@scop
Copy link
Owner

scop commented May 29, 2022

Hmm, in that case I wonder if the syntax we use in the variables' values here throw off shellcheck's parser. That would be unfortunate as we probably wouldn't be able to rely on it much in the file after the position of the definitions.

@akinomyoga
Copy link
Collaborator Author

akinomyoga commented May 29, 2022

Hmm, in that case I wonder if the syntax we use in the variables' values here throw off shellcheck's parser.

I think the problem is not that shellcheck fails to parse the right-hand side of the assignment. The shellcheck error can be reproduced even by the following simple script:

$ cat test.sh
#!/usr/bin/env bash
function setv { eval "$1=\$2"; }
function func1 { local var="'"; }
function func2 { local var; setv var 1; echo $var; }
$ shellcheck test.sh

In test.sh line 3:
function func1 { local var="'"; }
                            ^-- SC2089: Quotes/backslashes will be treated literally. Use an array.


In test.sh line 4:
function func2 { local var; setv var 1; echo $var; }
                                             ^--^ SC2090: Quotes/backslashes in this variable will not be respected.

For more information:
  https://www.shellcheck.net/wiki/SC2089 -- Quotes/backslashes will be treate...
  https://www.shellcheck.net/wiki/SC2090 -- Quotes/backslashes in this variab...

@scop
Copy link
Owner

scop commented May 29, 2022

Ok, sounds like a shellcheck bug anyway, albeit a not that worrying one, and not really introduced or made worse by this PR, so feel free to merge at will.

Would you mind reporting the shellcheck issue upstream?

@akinomyoga
Copy link
Collaborator Author

akinomyoga commented May 29, 2022

Maybe this is not really a bug of shellcheck, but shellcheck tries to find the value of uninitialized var in func2 in the above example. For example, if we slightly modify func2 as follows, the errors go away:

function func2 { local var; var=1; echo $var; }

If shellcheck misses something, that's the fact that the local variable var of func1 is not used anywhere else because func1 doesn't call any other shell function. If you are going to report it to the shellcheck upstream, I think we can point it out there.


I'll later again check and merge this PR. Thank you!

@akinomyoga akinomyoga merged commit 021c030 into scop:master Jun 1, 2022
@akinomyoga akinomyoga deleted the _comp_dequote branch June 1, 2022 10:41
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