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

ShellCheck #24

Closed
Dmole opened this issue Jan 18, 2024 · 3 comments
Closed

ShellCheck #24

Dmole opened this issue Jan 18, 2024 · 3 comments

Comments

@Dmole
Copy link

Dmole commented Jan 18, 2024

Do you use ShellCheck in your IDE?

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

@jkool702
Copy link
Owner

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
Copy link
Author

Dmole commented Jan 21, 2024

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
Copy link
Owner

jkool702 commented Feb 4, 2024

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!

@jkool702 jkool702 closed this as completed Feb 4, 2024
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

No branches or pull requests

2 participants