redhat-plumbers-in-action / differential-shellcheck

🐚 GitHub Action for running ShellCheck differentially
GNU General Public License v3.0
55 stars 9 forks source link

Shown alerts should include a possible solution (if available) #88

Open mrc0mmand opened 2 years ago

mrc0mmand commented 2 years ago

Type of issue

Feature Request

Description

ShellCheck provides possible solutions for reported issues in the Did you mean section, e.g.:

In /github/workspace/src/partition/test-repart.sh line 250:
JSON_OUTPUT=$("$repart" --definitions="$D/definitions-overrides" --dry-run=yes --empty=create --size=100M --json=pretty $D/test-drop-in-image)
                                                                                                                        ^-- SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
JSON_OUTPUT=$("$repart" --definitions="$D/definitions-overrides" --dry-run=yes --empty=create --size=100M --json=pretty "$D"/test-drop-in-image)

which the currently shown alerts don't include:

image

Describe the solution you'd like

Having the suggested solution as part of the alert would be great, maybe even in some diff-aware form, so the difference is clearly visible.

jamacku commented 2 years ago

We are aware of this issue. @lzaoral is already working on solution in:

Sorry for the inconvenience.

lzaoral commented 2 years ago

csutils/csdiff#68 is a different issue. csdiff relies on ShellCheck's GCC output formatter which, unfortunately, does not include suggested fixes. [1]

[1] https://www.shellcheck.net/wiki/Integration#GCC-compatible-error-messages

jamacku commented 1 year ago

I'm currently working on getting sarif-fmt package to Fedora. It could improve the current situation once available.

Example of output of sarif-fmt for ShellCheck:

$ shellcheck --format=json shell.sh | shellcheck-sarif | sarif-fmt
note: Use $(...) notation instead of legacy backticks `...`.
   ┌─ innocent-script.sh:15:15
   │
15 │ combined_file=`cat ${files}`
   │               ^^^^^^^^^^^^^^
   │
   ┌─ innocent-script.sh:15:15
   │
15 │ combined_file=`cat ${files}`
   │               -
   │
   ┌─ innocent-script.sh:15:28
   │
15 │ combined_file=`cat ${files}`
   │                            -
   │
   = SC2006
   = For more information: https://www.shellcheck.net/wiki/SC2006

Note: to make it work, we have to start using ShellCheck JSON format instead of GCC since GCC format is missing important information about fixes and defect position

jamacku commented 7 months ago

Following PR should make the situation a bit better:

jamacku commented 7 months ago

Possible solution would be implementation of:

mpoberezhniy commented 7 months ago

There is an action to add suggestions based on a git diff: reviewdog/action-suggester. It already implements filtering changes related to a change set so it can be used with shellcheck directly. It modifies all the lines that are available in PR context (as diff by default shows 3 lines of context and GitHub allows commenting on those lines).

I think having a write option in differential-shellcheck may be beneficial for local development when dealing with large files. It would allow applying the fixes only to the changed lines and allow using differential-shellcheck action with action-suggester.