Skip to content

Optimistically match shellcheck errors to source #556

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

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

Conversation

dpsutton
Copy link

@dpsutton dpsutton commented Aug 2, 2025

Addresses #360

The yaml ast node exposes if the string nodes are literals or not. If so, we can preserve shellcheck errors to their source in the yaml block rather than just the runs: key.

Example from sqlite-jdbc:

      - name: Build matrix from Makefile
        id: set-matrix
        # parse the Makefile to retrieve the list of targets in 'native-all', without 'native'
        run: |
          matrix=$((
            echo '{ "target" : ['
            sed -n "/^native-all *: */ { s///; p }" Makefile | sed "s/^native\s//g" | sed 's/ /, /g' | xargs -n 1 echo | sed -r 's/^([^,]*)(,?)$/"\1"\2/'
            echo " ]}"
          ) | jq -c .)
          echo $matrix | jq .
          echo "matrix=$matrix" >> $GITHUB_OUTPUT

output of

❯ actionlint -version
v1.7.7
installed by building from source
built with go1.21.6 compiler for darwin/arm64
.github/workflows/build-native.yml:68:9: shellcheck reported issue in this script: SC1102:error:1:8: Shells disambiguate $(( differently or not at all. For $(command substitution), add space after $( . For $((arithmetics)), fix parsing errors [shellcheck]
   |
68 |         run: |
   |         ^~~~
.github/workflows/build-native.yml:68:9: shellcheck reported issue in this script: SC2086:info:6:6: Double quote to prevent globbing and word splitting [shellcheck]
   |
68 |         run: |
   |         ^~~~
.github/workflows/build-native.yml:68:9: shellcheck reported issue in this script: SC2086:info:7:26: Double quote to prevent globbing and word splitting [shellcheck]
   |
68 |         run: |
   |         ^~~~

Output from this branch:

.github/workflows/build-native.yml:69:18: shellcheck reported issue in this script: SC1102:error:1:8: Shells disambiguate $(( differently or not at all. For $(command substitution), add space after $( . For $((arithmetics)), fix parsing errors [shellcheck]
   |
69 |           matrix=$((
   |                  ^~~
.github/workflows/build-native.yml:74:16: shellcheck reported issue in this script: SC2086:info:6:6: Double quote to prevent globbing and word splitting [shellcheck]
   |
74 |           echo $matrix | jq .
   |                ^~~~~~~
.github/workflows/build-native.yml:75:36: shellcheck reported issue in this script: SC2086:info:7:26: Double quote to prevent globbing and word splitting [shellcheck]
   |
75 |           echo "matrix=$matrix" >> $GITHUB_OUTPUT
   |                                    ^~~~~~~~~~~~~~

The difference becomes incredibly stark with greater than 10 errors. The existing output feels empty.

i don't beleive this is ready to merge as is. I'm a newcomer to the go language so I expect the code is not idiomatic. This project also has excellent testing coverage and documentation that this changeset does not come close to as is.

My first question is if you are open to this change. If so, I am very interseted in your thoughts to get this to the quality you like. Things that might be of interest:

  • put this feature behind a flag
  • gather shell scripts from top 1000 github repos and see the results similar to the popular actions
  • extensive tests in testdata with expected output
  • update the documentation examples to generate output
  • extend this to pyflakes mechanism

Thank you for the project

Screenshots to give visceral feel

sqlite-jdbc

current
image
proposed
image

liquibase

current
image
proposed
image

druid

current
image
proposed
image

The yaml ast node exposes if the string nodes are literals or not. If
so, we can preserve shellcheck errors to their source in the yaml block
rather than just the `runs:` key.

Example from sqlite-jdbc:

```yaml
      - name: Build matrix from Makefile
        id: set-matrix
        # parse the Makefile to retrieve the list of targets in 'native-all', without 'native'
        run: |
          matrix=$((
            echo '{ "target" : ['
            sed -n "/^native-all *: */ { s///; p }" Makefile | sed "s/^native\s//g" | sed 's/ /, /g' | xargs -n 1 echo | sed -r 's/^([^,]*)(,?)$/"\1"\2/'
            echo " ]}"
          ) | jq -c .)
          echo $matrix | jq .
          echo "matrix=$matrix" >> $GITHUB_OUTPUT
```

output of

```
❯ actionlint -version
v1.7.7
installed by building from source
built with go1.21.6 compiler for darwin/arm64
```

```
.github/workflows/build-native.yml:68:9: shellcheck reported issue in this script: SC1102:error:1:8: Shells disambiguate $(( differently or not at all. For $(command substitution), add space after $( . For $((arithmetics)), fix parsing errors [shellcheck]
   |
68 |         run: |
   |         ^~~~
.github/workflows/build-native.yml:68:9: shellcheck reported issue in this script: SC2086:info:6:6: Double quote to prevent globbing and word splitting [shellcheck]
   |
68 |         run: |
   |         ^~~~
.github/workflows/build-native.yml:68:9: shellcheck reported issue in this script: SC2086:info:7:26: Double quote to prevent globbing and word splitting [shellcheck]
   |
68 |         run: |
   |         ^~~~
```

Output from this branch:

```
.github/workflows/build-native.yml:69:18: shellcheck reported issue in this script: SC1102:error:1:8: Shells disambiguate $(( differently or not at all. For $(command substitution), add space after $( . For $((arithmetics)), fix parsing errors [shellcheck]
   |
69 |           matrix=$((
   |                  ^~~
.github/workflows/build-native.yml:74:16: shellcheck reported issue in this script: SC2086:info:6:6: Double quote to prevent globbing and word splitting [shellcheck]
   |
74 |           echo $matrix | jq .
   |                ^~~~~~~
.github/workflows/build-native.yml:75:36: shellcheck reported issue in this script: SC2086:info:7:26: Double quote to prevent globbing and word splitting [shellcheck]
   |
75 |           echo "matrix=$matrix" >> $GITHUB_OUTPUT
   |                                    ^~~~~~~~~~~~~~
```

The difference becomes incredibly stark with greater than 10 errors. The
existing output feels empty.

i don't beleive this is ready to merge as is. I'm a newcomer to the go
language so I expect the code is not idiomatic. This project also has
excellent testing coverage and documentation that this changeset does
not come close to as is.

My first question is if you are open to this change. If so, I am very
interseted in your thoughts to get this to the quality you like. Things
that might be of interest:
- put this feature behind a flag
- gather shell scripts from top 1000 github repos and see the results
similar to the popular actions
- extensive tests in testdata with expected output
- update the documentation examples to generate output
- extend this to pyflakes mechanism

Thank you for the project
@dpsutton dpsutton marked this pull request as draft August 2, 2025 21:55
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.

1 participant