Drop #shellcheck comments

[jasp00]

Follows #3455.

Scripts should not have # shellcheck disable comments and multiple commands in .travis.yml should be moved to separate scripts that could be checked.

[tresf]

Please elaborate. @zapashcanon and myself both feel #3455 is in good shape. Please specifically point out the major problem that the PR creates.

[zapashcanon]

multiple commands in .travis.yml should be moved to separate scripts that could be checked.

Done in #3470.

[jasp00]

Please specifically point out the major problem that the PR creates.

While the PR works and it is a fine addition, it introduces the need for # shellcheck disable; a smart tool would not need such comments. The more or less important disadvantages are:

1. Readability, the comment does not explain the code.
2. Dependency on a checking tool.
3. A newcomer must know (and an old developer must remember) that such comment is needed for common and legal uses of variables.

[tresf]

Readability, the comment does not explain the code.

This happens fairly often in programming languages (CMAKE_POLICY, @SupressWarnings, #pragma) , bash just doesn't have a better way to do it. Adding more scripts is not the answer.

Dependency on a checking tool.

This script has no dependency on any checking tools. This argument is invalid.

A newcomer must know (and an old developer must remember) that such comment is needed for common and legal uses of variables.

If not, Travis will (eventually) warn them once we get it into our CI. This is intended.

Please provide a better argument. The time spent discussing this topic adds little value. It should be put in more useful places.

[jasp00]

This happens fairly often in programming languages

Which does not make it less of a disadvantage. You are citing workarounds, CMAKE_POLICY is a temporary solution while a real fix is being developed.

Adding more scripts is not the answer.

Answer to what?

This script has no dependency on any checking tools.

What happens if you remove the comment?

Travis will (eventually) warn them once we get it into our CI.

May you post the shellcheck error?

This is intended.

This error could be avoided if shellcheck were smarter, so I am proposing an enhancement.

[tresf]

This error could be avoided if shellcheck were smarter, so I am proposing an enhancement.

Complain to shellcheck. This is staying as-is.

[jasp00]

Complain to shellcheck.

Someone may.

This is staying as-is.

Why? You could disable these checks from the command-line invocation.

@zapashcanon, may you post the shellcheck error?

[zapashcanon]

@jasp00: not sure if this is what you're asking but:

SC2086: Double quote to prevent globbing and word splitting.
(plus a little ^ showing which var it's talking about)

SC1090: Can't follow non-constant source. Use a directive to specify location.
(plus a little ^ showing which var it's talking about)

From their wiki. They have a very good wiki I have to say. If you just google SCxxxx you'll find a complete explanation, so I don't think this would be a problem.

You can also see it the the travis log.

[jasp00]

Thank you, @zapashcanon. As I thought, messages give no clue about how to disable these errors, which come from shellcheck's limitations rather than the code. Because of mentioned disadvantages, these checks should be disabled from the command line to decouple scripts from the specific tool.

[zapashcanon]

So you'd rather disable this warning and miss a lot more of cases where quoting is necessary ?

[tresf]

Stop this madness.

[jasp00]

@tresf, could you explain why cannot this enhancement be achieved? Dropping these comments would help.

So you'd rather disable this warning and miss a lot more of cases where quoting is necessary ?

I am concerned that a newcomer may try to fix a false positive, without a hint that it may be so, and go unnoticed; she may not look for SC2086 and may double quote as instructed. Thus, I have suggested to disable the warning. This was my first impression, now I am not so sure.

Should we disable these warnings? I only say that shellcheck should be smarter as I indicated, then these comments would not be needed. I did not consider SC1090, which looks more tricky, but I guess it can be solved; then these comments would not be needed either.

You do not have to drop comments right now, since workarounds are temporarily necessary, but it should be a goal that someone may try to accomplish later.

[tresf]

You do not have to drop comments right now, since workarounds are temporarily necessary, but it should be a goal that someone may try to accomplish later.

You're bikeshedding. If someone finds a clean way to remove the comments without adding unnecessary complications to these very simple shell scripts, we'd happily merge.

[jasp00]

You're bikeshedding. If someone finds a clean way to remove the comments without adding unnecessary complications to these very simple shell scripts, we'd happily merge.

Yet you have closed the issue, so no one will look for a clean way.