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 github status check for shell check #482

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

IgorTodorovskiIBM
Copy link
Collaborator

@IgorTodorovskiIBM IgorTodorovskiIBM commented Oct 5, 2023

Copy link
Collaborator

@DevonianTeuchter DevonianTeuchter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Collaborator

@MikeFultonDev MikeFultonDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@IgorTodorovskiIBM
Copy link
Collaborator Author

Found this: https://github.com/marketplace/actions/sh-checker it includes shfmt as well as shellcheck

@IgorTodorovskiIBM IgorTodorovskiIBM marked this pull request as draft October 6, 2023 19:39
@v1gnesh
Copy link
Collaborator

v1gnesh commented Oct 7, 2023

Wondering if shfmt could be used as a pre-commit hook instead.
This way, we get to see what changes it'll make before committing.
Also, VS Code and Jetbrains IDEs have integration to set this up.

https://github.com/scop/pre-commit-shfmt/tree/main

Similarly, both these IDEs have plugins for shellcheck; so can offer corrective actions while developing.
Correction is one click away. Action for shellcheck is still needed as it produces useful review comments.
As time goes and all zopen devs use shellcheck and shfmt locally (ideally), this Action can serve to make it easy for newer contributors and as a general fallback.

@MikeFultonDev
Copy link
Collaborator

That makes sense.
Are people comfortable it won’t introduce any bashisms? I don’t want to pre-req bash (yet at least) for our scripts.

@IgorTodorovskiIBM
Copy link
Collaborator Author

A pre-commit hook for shfmt is a good idea

That makes sense. Are people comfortable it won’t introduce any bashisms? I don’t want to pre-req bash (yet at least) for our scripts.

It's just a formatter so it should be ok, all the tools seem to work okay from my tests. shellcheck on the other hand can change the semantics of the program as I've unfortunately experienced, so I've had to disable some options here https://github.com/ZOSOpenTools/meta/pull/486/files#diff-3648cd4f7d46f434c6e6e21a44d23f5716574948ed690d79452535f1abf2ddb8R50

Printer options:

  -i,  --indent uint       0 for tabs (default), >0 for number of spaces
  -bn, --binary-next-line  binary ops like && and | may start a line
  -ci, --case-indent       switch cases will be indented
  -sr, --space-redirects   redirect operators will be followed by a space
  -kp, --keep-padding      keep column alignment paddings
  -fn, --func-next-line    function opening braces are placed on a separate line

@v1gnesh
Copy link
Collaborator

v1gnesh commented Oct 8, 2023

For vim users, there are plugins for both shfmt and shellcheck.
https://github.com/z0mbix/vim-shfmt
https://github.com/itspriddle/vim-shellcheck
I haven't tested either..
Also, vim 8.x onwards comes with an in-built plugin manager, so no other dependencies needed.

Copy link
Collaborator

@MikeFultonDev MikeFultonDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did not mean to approve...

Copy link
Collaborator

@MikeFultonDev MikeFultonDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a draft - ignore my approval 😁

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.

4 participants