Prefer well defined expressions

[spotlesstofu]

• Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
• Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.

https://stackoverflow.com/questions/34755698/why-are-and-preferred-to-a-and-o

[smoser]

Hi,

I'm not currently the primary maintainer of cloud-utils, but I would like to offer my opinion on this change.

First I'd like to say that shellcheck is a very useful lint tool.

Second, with this change specifically:

https://stackoverflow.com/questions/34755698/why-are-and-preferred-to-a-and-o

I disagree with this for at least 2 reasons:

1. Obvious "correctness" s would say that '[ p -a q ]' is the same as[ p ] && [ q ]. One should not need a formal posix definition to expect that.
2. 100% of shells I have ever used behave in this way.

If you have a relevant example of where this code does not work as intended, please give example. We should absolutely fix it if it does not work as expected. My feeling is that there is cost but no value attached to changing code to accommodate either hypothetical situation or shell that is not relevant or used any where (even if that shell is arguably "compliant" to a standard).

So my feeling is if you're going to apply 'lint' fixes to this code:
a. run the lint in c-i, otherwise it is useless
b. in shellcheck lint, at ignore at least SC2015,SC2166,SC3043,SC2162

[spotlesstofu]

@smoser thanks for your review & advice. I don't have an example. I just figured this change could make the code more robust.

[paride]

Hi @smoser and thanks @spotlesstofu for opening this PR.

a. run the lint in c-i, otherwise it is useless

shellcheck does run in c-i, but with --severity=error, so SC2166 doesn't show up (see tox.ini).

On SC2166 (i.e. [ p ] && [ q ] vs [ p -a q ]): the shellcheck wiki says that the latter can cause incorrect results when arguments start with dashes or contain ! (https://www.shellcheck.net/wiki/SC2166). This said, I have never encountered the issue in practice either, but I also believe that there is some value in following this kind "maybe useless but also not harmful" linter suggestions, as they provide a useful baseline, moreover if they did implement the check is most likely means that as someone got bitten at some point.

Anyway, @spotlesstofu you are right in that it would be good to improve the shellcheck compliance of the scripts. If you want to do so my suggestion here is to:

1. Switch to --severity=error to warning in tox.ini. Run tox -e shellcheck: it should issue several warning.
2. Fix what you can, possibly fixing different kinds of warning in separate commits.
3. Discuss non obvious cases (e.g. SC2039, having local is useful)
4. Add tags we don't want to fix in .shellcheckrc
5. Verify that CI passes