jkool702/forkrun

ShellCheck

Dmole opened this issue · 3 comments

Dmole commented

Do you use ShellCheck in your IDE?

It has some issues with this code that should be fixed or marked as an exception.

jkool702 commented

Yes, ShellCheck was used (fairly extensively) as I was writing forkrun.

All the shellcheck warnings shown fall into one of four categories:

  1. stylistic choices. notably, [SC2004](https://www.shellcheck.net/wiki/SC2004) (style): $/${} is unnecessary on arithmetic variables.
  2. shellcheck not being advanced enough to understand the code. In particular the few places that forkrun d namically generates code qand then sources it give it trouble. notably [SC1090](https://www.shellcheck.net/wiki/SC1090) (warning): ShellCheck can't follow non-constant source. Use a directive to specify location.. The [SC2034](https://www.shellcheck.net/wiki/SC2034) (warning): <variable> appears unused. Verify use (or export if used externally). warnings directly stem from that as well.
  3. What shellcheck is warning about is intentional behavior. Notably [SC2016](https://www.shellcheck.net/wiki/SC2016) (info): Expressions don't expand in single quotes, use double quotes for that.
  4. What shellcheck is warning about can never occur and thus isnt a problem. This includes warnings like [SC2162](https://www.shellcheck.net/wiki/SC2162) (info): read without -r will mangle backslashes. when what is being read is guaranteed to be just a number without spaces/backslashes/anything other than digits.

I could safely disable SC2004 and SC1090, as these particular rules can always be safely ignored, but the rest are cases where in that specific situation the rule doesnt apply, not cases where the rule should unilaterally never apply, so I wouldnt want to disable those.

Dmole commented

If you want you can just add a comment like

# shellcheck disable=SC2016

And it will only impact the following line/block

(Having zero warnings is the easiest way to notice when a new unintentional warning occurs)

jkool702 commented

I just pushed a update to the main branch that bumps the version to forkrun v1.1. The main new feature is support for splitting up stdin based on the number of bytes read (supported by 2 new flags: -b and -B), but one of the smaller miscellaneous changers was to globally disable many of the not-really-useful/applicable shellcheck warnings.

I havent gone through and line-by-line disabled the 20 or so shellcheck warnings that are useful warnings in general but dont apply to that specific line. At some point once forkrun is more-or-less "stable and complete" and not still having new features actively developed Ill probably add these line-by-line shellcheck disable directives.

Im going to go ahead and close this issue. Thanks for taking the time to submit it!