rcaloras / bash-preexec

⚡ preexec and precmd functions for Bash just like Zsh.
MIT License
862 stars 94 forks source link

Fix the test for the Bash version for shellcheck #142

Closed akinomyoga closed 1 year ago

akinomyoga commented 1 year ago

I have separated this commit from #141 as suggested by @dimo414 (https://github.com/rcaloras/bash-preexec/pull/141#discussion_r1127193800).

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

Quoted from https://github.com/rcaloras/bash-preexec/pull/141#issue-1612395608

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 https://github.com/rcaloras/bash-preexec/pull/141#discussion_r1127198702

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Thanks for the additional commit!

rcaloras commented 1 year ago

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

akinomyoga commented 1 year ago

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.