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

Add auto-formatting script and shellcheck static analysis with workflows. #293

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ZeroEcks
Copy link

@ZeroEcks ZeroEcks commented Oct 5, 2024

Hi, as we briefly discussed in #289, I think that it could be beneficial to the repository to have two things

  • Static Analysis of potential issues with the shellscripts. Shellcheck provides some awesome stuff, as you can see in their gallery of bad code such as; catching mistakes with conditionals, poor quoting, and heaps of others. My favourite is the common 'catastropic rm' (rm -rf "$STEAMROOT/"* which would delete /* in the event of an undefined variable). IMO ShellCheck is essential for all shell scripts as it can really save you from a footgun.
  • Auto formatting with shfmt. This adds the ability to quickly auto format all the shell files in the repository to follow a common standard. This would be helpful for me particularly, as I would be able to ensure I don't have minor whitespace differences between my fork and upstream.

I have included two things

  • ./fmt.sh which provides some simple utilities for formatting, checking and auto application

  • Two Github Actions workflows

    • Shellcheck: that will alert you on PRs to new shellcheck warnings introduced, and add code scanning results to your security dashboard. See here for an example..
    • shfmt: this will comment on new PRs with shfmt errors. Please note, you will have to fix all of them on main... or it will complain quite a lot on every PR. I haven't included one that scans main.

    Please note, there is still a bunch of shellcheck issues I did not autofix, mostly because I would like to know if these are welcome changes before I spend time resolving them :)

@ZeroEcks
Copy link
Author

ZeroEcks commented Oct 5, 2024

I can also revert the autoformat and autofix commits so you can commit them yourself if you prefer also.

I also did not mention, I recommend using a shellcheck editor extension as it can help avoid a lot of issues, even if you don't adopt it as part of the CI or anything.

shfmt also just uses the .editorconfig standard to determine how to format the files, I added one in that seemed close enough to what you already used. Feel free to adjust as you see fit and I can reset the formatting commits (best to keep those to a minimum). There is also extensions available for shfmt for most editors.

@mylinuxforwork
Copy link
Owner

@ZeroEcks Thank you so much for highlighting this topic up. It is super interesting and valuable for the whole project.
I want to learn more about. It’s needed to implement these quality checks on my side.

Is it OK to keep you posted about my learning progress and not merging it right now?

@ZeroEcks
Copy link
Author

ZeroEcks commented Oct 5, 2024

Totally fine! the shellcheck wiki can be good for learning about the common mistakes in shell scripts. Feel free to email me any questions.

@mylinuxforwork
Copy link
Owner

@ZeroEcks Thank you so much for your help.

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

Successfully merging this pull request may close these issues.

2 participants