Skip to content

Revert "Remove non-working problem matchers. (#88)" #103

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.

Sign up for GitHub

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

Open
Root-Core wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Revert "Remove non-working problem matchers. (#88)" #103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions .github/problem-matcher-gcc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{
"problemMatcher": [
{
"owner": "shellcheck-gcc",
"pattern": [
{
"regexp": "^\\.?\\/?(.+):(\\d+):(\\d+):\\s(warning|error):\\s(.*)\\s\\[(SC\\d+)\\]$",
"file": 1,
"line": 2,
"column": 3,
"severity": 4,
"message": 5,
"code": 6
}
]
},
{
"owner": "shellcheck-gcc-notice",
"severity": "notice",
"pattern": [
{
"regexp": "^\\.?\\/?(.+):(\\d+):(\\d+):\\s(note):\\s(.*)\\s\\[(SC\\d+)\\]$",
"file": 1,
"line": 2,
"column": 3,
"message": 5,
"code": 6
}
]
}
]
}
Comment on lines +1 to +32
Copy link

@coderabbitai coderabbitai bot Oct 14, 2024

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Well-structured problem matcher configuration for shellcheck output.

The overall structure of this file is excellent, providing comprehensive coverage for shellcheck output in GCC format. The separation of matchers for warnings/errors and notices enhances the precision of issue categorization in GitHub Actions.

For improved maintainability, consider adding a brief comment at the top of the file explaining its purpose and the format of shellcheck output it's designed to parse. This would help future maintainers quickly understand the file's role.

23 changes: 23 additions & 0 deletions .github/problem-matcher-tty.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
"problemMatcher": [
{
"owner": "shellcheck-tty",
"pattern": [
{
"regexp": "^In\\s\\.?\\/?(.+)\\sline\\s(\\d+):$",
"file": 1,
"line": 2
},
{
"regexp": ".*"
},
{
"regexp": "(SC\\d+):\\s(.+)$",
"code": 1,
"message": 2,
"loop": true
}
Comment on lines +14 to +19
Copy link

@coderabbitai coderabbitai bot Oct 14, 2024

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: Effective pattern for capturing shellcheck warnings with a minor suggestion.

The regular expression (SC\\d+):\\s(.+)$ effectively captures the shellcheck code and message. The "loop" property is correctly set to true, allowing multiple warnings to be captured.

Consider adding word boundaries to the shellcheck code pattern for increased precision:

-    "regexp": "(SC\\d+):\\s(.+)$",
+    "regexp": "\\b(SC\\d+):\\s(.+)$",

This change ensures that the pattern only matches "SC" when it's at the start of a word, potentially preventing false positives.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{
"regexp": "(SC\\d+):\\s(.+)$",
"code": 1,
"message": 2,
"loop": true
}
{
"regexp": "\\b(SC\\d+):\\s(.+)$",
"code": 1,
"message": 2,
"loop": true
}

]
}
]
}
Comment on lines +1 to +23
Copy link

@coderabbitai coderabbitai bot Oct 14, 2024

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Overall assessment: Well-designed problem matcher configuration.

This problem matcher configuration for shellcheck-tty output is well-structured and should effectively integrate shellcheck warnings into the GitHub Actions workflow. It correctly captures file names, line numbers, shellcheck codes, and warning messages.

Key points:

  1. The structure follows the expected format for GitHub Actions problem matchers.
  2. The patterns effectively parse shellcheck output.
  3. The use of the "loop" property allows for capturing multiple warnings.

Consider addressing the following minor points:

  1. Clarify the purpose of the catch-all pattern (.*).
  2. Consider adding word boundaries to the shellcheck code pattern for increased precision.

This problem matcher will enhance the visibility and tracking of shellcheck issues within your GitHub Actions workflow, improving the overall code quality process.

18 changes: 16 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,12 +140,26 @@ If you need to scan for unusual files, you can use the `additional_files` key.
additional_files: 'run finish'
```

## Disable problem matcher

If you do not want to have the problem-matcher annotate files, you can disable it
by setting `disable_matcher` to `true`.

```yaml
...
- name: Run ShellCheck
uses: ludeeus/action-shellcheck@master
with:
disable_matcher: true
```
Comment on lines +143 to +154
Copy link

@coderabbitai coderabbitai bot Oct 14, 2024

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM! Consider adding a brief explanation of the problem matcher.

The new section about disabling the problem matcher is well-structured and provides a clear example of how to use the disable_matcher option. This addition aligns with the PR objectives and enhances the configurability of the action.

To further improve clarity, consider adding a brief explanation of what the problem matcher does and why a user might want to disable it. For example:

 ## Disable problem matcher

-If you do not want to have the problem-matcher annotate files, you can disable it
+The problem matcher automatically annotates files with issues found by ShellCheck.
+If you do not want to have these automatic annotations, you can disable the matcher
 by setting `disable_matcher` to `true`.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
## Disable problem matcher
If you do not want to have the problem-matcher annotate files, you can disable it
by setting `disable_matcher` to `true`.
```yaml
...
- name: Run ShellCheck
uses: ludeeus/action-shellcheck@master
with:
disable_matcher: true
```
## Disable problem matcher
The problem matcher automatically annotates files with issues found by ShellCheck.
If you do not want to have these automatic annotations, you can disable the matcher
by setting `disable_matcher` to `true`.
```yaml
...
- name: Run ShellCheck
uses: ludeeus/action-shellcheck@master
with:
disable_matcher: true
```


## Change output format

Shellcheck can print output in these formats: `checkstyle`, `diff`, `gcc`, `json`, `json1`, `quiet`, `tty`. See some examples [here](https://github.com/koalaman/shellcheck/wiki/Integration#pick-the-output-format-that-makes-your-life-easier).
Only `tty` and `gcc` produce file annotations via problem matcher, default is `gcc`.

- `tty` has multi-line log messages
- `gcc` has single-line log messages
- `tty` has multi-line log messages, but all annotations are reported as errors
- `gcc` has single-line log messages, so it's easier to parse with a problem matcher (including correct severity annotation)
Comment on lines +159 to +162
Copy link

@coderabbitai coderabbitai bot Oct 14, 2024

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM! Consider standardizing the format of the list items.

The changes to the output format section provide valuable clarification on the behavior of 'tty' and 'gcc' formats. This information helps users make an informed decision about which format to use based on their needs.

To improve consistency with the rest of the document, consider standardizing the format of the list items. For example:

-Only `tty` and `gcc` produce file annotations via problem matcher, default is `gcc`.
+Only `tty` and `gcc` produce file annotations via problem matcher. The default is `gcc`.

-- `tty` has multi-line log messages, but all annotations are reported as errors
-- `gcc` has single-line log messages, so it's easier to parse with a problem matcher (including correct severity annotation)
+- `tty`: Has multi-line log messages, but all annotations are reported as errors.
+- `gcc`: Has single-line log messages, making it easier to parse with a problem matcher (including correct severity annotation).
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Only `tty` and `gcc` produce file annotations via problem matcher, default is `gcc`.
- `tty` has multi-line log messages
- `gcc` has single-line log messages
- `tty` has multi-line log messages, but all annotations are reported as errors
- `gcc` has single-line log messages, so it's easier to parse with a problem matcher (including correct severity annotation)
Only `tty` and `gcc` produce file annotations via problem matcher. The default is `gcc`.
- `tty`: Has multi-line log messages, but all annotations are reported as errors.
- `gcc`: Has single-line log messages, making it easier to parse with a problem matcher (including correct severity annotation).


```yaml
...
Expand Down
19 changes: 18 additions & 1 deletion action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ inputs:
description: "Set to true to skip using problem-matcher"
required: false
default: "false"
deprecationMessage: "There are no problem-matchers, this setting does not do anything."
format:
description: "Output format (checkstyle, diff, gcc, json, json1, quiet, tty)"
required: false
Expand All @@ -57,6 +56,17 @@ branding:
runs:
using: "composite"
steps:
- name: Enable problem-matcher
shell: bash
env:
INPUT_FORMAT: ${{ inputs.format }}
INPUT_DISABLE_MATCHER: ${{ inputs.disable_matcher }}
run: |
problem_matcher_file="${{ github.action_path }}/.github/problem-matcher-${INPUT_FORMAT}.json"
if [[ "${INPUT_DISABLE_MATCHER}" != "true" && -f "$problem_matcher_file" ]]; then
echo "::add-matcher::$problem_matcher_file"
fi

Comment on lines +59 to +69
Copy link

@coderabbitai coderabbitai bot Oct 14, 2024

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM! Consider adding a comment for clarity.

The new "Enable problem-matcher" step effectively reintroduces the problem matcher functionality, aligning with the PR objectives. The logic is sound, checking for both the disable flag and the existence of the matcher file.

Consider adding a brief comment explaining the purpose of this step, e.g.:

# Enable problem-matcher if not disabled and the matcher file exists

This would enhance readability and maintainability.

- name: Download shellcheck
shell: bash
env:
Expand Down Expand Up @@ -220,6 +230,13 @@ runs:

set +f # re-enable globbing

- name: Remove problem-matcher
shell: bash
run: |
echo "::remove-matcher owner=shellcheck-gcc::"
echo "::remove-matcher owner=shellcheck-gcc-notice::"
echo "::remove-matcher owner=shellcheck-tty::"

Comment on lines +233 to +239
Copy link

@coderabbitai coderabbitai bot Oct 14, 2024

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM! Consider using environment variables for consistency.

The new "Remove problem-matcher" step effectively cleans up the problem matchers added during the action's execution. This is a good practice to avoid potential conflicts with other actions.

For consistency with the "Enable problem-matcher" step, consider using environment variables for the matcher names:

- name: Remove problem-matcher
  shell: bash
  env:
    MATCHER_GCC: "shellcheck-gcc"
    MATCHER_GCC_NOTICE: "shellcheck-gcc-notice"
    MATCHER_TTY: "shellcheck-tty"
  run: |
    echo "::remove-matcher owner=${MATCHER_GCC}::"
    echo "::remove-matcher owner=${MATCHER_GCC_NOTICE}::"
    echo "::remove-matcher owner=${MATCHER_TTY}::"

This approach would make it easier to update matcher names in the future if needed.

- name: Exit action
shell: bash
run: exit ${{steps.check.outputs.statuscode}}