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

Fix the test for the Bash version for shellcheck #142

Merged
merged 2 commits into from
Mar 13, 2023

Conversation

akinomyoga
Copy link
Contributor

I have separated this commit from #141 as suggested by @dimo414 (#141 (comment)).

As in the cover of #141, we seem to need to update the version check due to the shellcheck updates:

Quoted from #141 (comment)

ShellCheck seems to have started to complain about -lt, -eq, etc. in the conditional command [[ ... ]], so I also included a commit (77aec5f) to workaround the shellcheck warning.

Also

Quoted from #141 (comment)

I have introduced the version check in #136. At that time, I just used the single conditional command [[ ... ]] because it looked simpler to me compare to combining [[ ... ]] || (( ... )). This time, I have checked the updated version check works as expected with Bash 1.14.7, 2.0, 3.0, and 3.1. Bash 1.14.7 doesn't have BASH_VERSINFO, so it is rejected by [[ -z "${BASH_VERSINFO-}" ]]. Bash 2.0+ has an arithmetic command (( ... )) and seems to work as expected.

@akinomyoga
Copy link
Contributor Author

Actually, another option would be to use # shellcheck disable=... to just suppress the shellcheck warning, which might be simpler in this case. What do you think?

@dimo414
Copy link
Collaborator

dimo414 commented Mar 7, 2023

Nah I definitely prefer arithmetic contexts, but swapping from the legacy pattern to them in code specifically checking for legacy environments is more risky than normal. As noted in the earlier thread in #141 "As you say very old versions of Bash support arithmetic contexts (2.0, according to https://mywiki.wooledge.org/BashFAQ/061) and the -z will short-circuit older versions." So I think this is safe.

@akinomyoga
Copy link
Contributor Author

akinomyoga commented Mar 7, 2023

Thanks for the additional commit!

@rcaloras
Copy link
Owner

Is this PR still needed as well? @dimo414 feel free to merge if needed.

@akinomyoga
Copy link
Contributor Author

For the current master of bash-preexec.sh, shellcheck 0.8.0 in my environment produces the following error:

In bash-preexec.sh line 46:
if [[ -z "${BASH_VERSINFO-}" || BASH_VERSINFO[0] -lt 3 || BASH_VERSINFO[0] -eq 3 && BASH_VERSINFO[1] -lt 1 ]]; then
                                ^--------------^ SC2309 (warning): -lt treats this as an arithmetic expression. Use < to compare as string (or expand explicitly with $((expr))).
                                                          ^--------------^ SC2309 (warning): -eq treats this as an arithmetic expression. Use = to compare as string (or expand explicitly with $((expr))).
                                                                                    ^--------------^ SC2309 (warning): -lt treats this as an arithmetic expression. Use < to compare as string (or expand explicitly with $((expr))).

For more information:
  https://www.shellcheck.net/wiki/SC2309 -- -eq treats this as an arithmetic ...

The same error seems to be produced also in the latest version of shellcheck at shellcheck.net.

@rcaloras rcaloras merged commit fb23d47 into rcaloras:master Mar 13, 2023
@akinomyoga akinomyoga deleted the shellcheck-BASH_VERSINFO branch March 13, 2023 00:58
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.

3 participants