Revert "Remove non-working problem matchers. (#88)" #103
Conversation
This reverts commit 00cae50.
📝 WalkthroughWalkthroughThe changes introduce new JSON configuration files for problem matching in the ShellCheck GitHub action, specifically for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub Action
participant ShellCheck
participant Problem Matcher
User->>GitHub Action: Trigger action
GitHub Action->>ShellCheck: Run ShellCheck
ShellCheck->>GitHub Action: Return output
GitHub Action->>Problem Matcher: Enable matcher (if not disabled)
Problem Matcher->>GitHub Action: Parse output
GitHub Action->>User: Display results
GitHub Action->>Problem Matcher: Remove matcher
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (1)
action.yaml (1)
Line range hint
9-25: LGTM! Consider updating usage examples in README.The updates to input parameters improve the action's functionality and clarity:
- Deprecating
ignorein favor ofignore_pathsandignore_namesprovides more specific control.- The updated description for
disable_matcheraccurately reflects its current functionality.Consider updating the README or documentation to include usage examples for the new
ignore_pathsandignore_namesparameters, helping users transition from the deprecatedignoreparameter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- .github/problem-matcher-gcc.json (1 hunks)
- .github/problem-matcher-tty.json (1 hunks)
- README.md (1 hunks)
- action.yaml (2 hunks)
🧰 Additional context used
🔇 Additional comments (7)
.github/problem-matcher-tty.json (3)
1-23: LGTM: Well-structured problem matcher configuration.The overall structure of the problem matcher configuration is correct and follows the expected format for GitHub Actions problem matchers.
6-10: LGTM: Effective pattern for capturing file and line information.The regular expression
^In\\s\\.?\\/?(.+)\\sline\\s(\\d+):$effectively captures the file name and line number from shellcheck output. It correctly handles cases where the file path might start with "./" or "/".
11-13: Clarify the purpose of the catch-all pattern.The second pattern uses the regular expression
.*, which matches any string. While this can be useful for skipping a line in the output, it's worth clarifying its specific purpose in this context. Is it intended to consume a line of output that isn't needed for problem matching?Could you please explain the rationale behind this catch-all pattern?
.github/problem-matcher-gcc.json (2)
3-16: Well-structured problem matcher for shellcheck warnings and errors.The "shellcheck-gcc" matcher is effectively designed to capture warnings and errors from shellcheck output. The regular expression is comprehensive, allowing for both relative and absolute file paths, and accurately capturing all necessary information (file, line, column, severity, message, and code).
17-30: Consistent and well-designed problem matcher for shellcheck notices.The "shellcheck-gcc-notice" matcher complements the previous matcher by specifically targeting notices. It maintains consistency in handling file paths and capturing relevant information. The hardcoded severity as "notice" is appropriate for this use case.
README.md (1)
143-162: Great improvements to the documentation!The changes made to the README.md file significantly enhance the documentation for the ShellCheck GitHub action. The new section on disabling the problem matcher and the clarifications to the output format section provide users with valuable information and configuration options.
These additions align well with the PR objectives of reverting the removal of problem matchers and improve the overall usability of the action. The changes are consistent with the existing documentation style and provide clear examples for users to follow.
Great job on improving the documentation!
action.yaml (1)
Line range hint
1-243: Overall, the changes look good and align with the PR objectives.The modifications to
action.yamlsuccessfully reintroduce the problem matcher functionality while improving the action's overall structure and clarity. Key improvements include:
- Addition of steps to enable and remove problem matchers.
- Deprecation of the
ignoreparameter in favor of more specificignore_pathsandignore_names.- Updated descriptions for input parameters to accurately reflect their functionality.
These changes enhance the action's usability and maintainability. The suggestions provided in the review comments are minor and aimed at further improving consistency and documentation.
| { | ||
| "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 | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
🧹 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:
- The structure follows the expected format for GitHub Actions problem matchers.
- The patterns effectively parse shellcheck output.
- The use of the "loop" property allows for capturing multiple warnings.
Consider addressing the following minor points:
- Clarify the purpose of the catch-all pattern (
.*). - 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.
| { | ||
| "regexp": "(SC\\d+):\\s(.+)$", | ||
| "code": 1, | ||
| "message": 2, | ||
| "loop": true | ||
| } |
There was a problem hiding this comment.
🧹 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.
| { | |
| "regexp": "(SC\\d+):\\s(.+)$", | |
| "code": 1, | |
| "message": 2, | |
| "loop": true | |
| } | |
| { | |
| "regexp": "\\b(SC\\d+):\\s(.+)$", | |
| "code": 1, | |
| "message": 2, | |
| "loop": true | |
| } |
| { | ||
| "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 | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
🧹 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.
| ## 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 | ||
| ``` |
There was a problem hiding this comment.
🧹 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.
| ## 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 | |
| ``` |
| 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) |
There was a problem hiding this comment.
🧹 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.
| 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). |
| - 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 | ||
|
|
There was a problem hiding this comment.
🧹 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 existsThis would enhance readability and maintainability.
| - 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::" | ||
|
|
There was a problem hiding this comment.
🧹 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.
|
Until GitHub documents this in their developer documentation; this is not something I want to add back. |
|
The https://github.com/actions/ organization is verified to be part of github.com and contains documentation about problem matchers: https://github.com/actions/toolkit/blob/main/docs/problem-matchers.md The feature hasn't really changed since its inception. The last change to the documentation was on I don't know the exact issue that caused the problem matcher to be removed, but it was most likely just a bug with GitHub. I understand your position, but please consider adding it back. It is a really useful feature. |
|
Tip For best results, initiate chat on the files or code changes. @Root-Core: Thank you for sharing the official documentation from the GitHub Actions Toolkit. The problem matcher feature is indeed stable and well-supported. Restoring the problem matchers can significantly enhance the usability of this action by providing immediate feedback and annotations directly in the GitHub UI. Considering that the previous removal might have been due to a temporary issue with GitHub, it seems reasonable to reinstate them. Including the problem matchers would greatly benefit users by highlighting issues in the code more effectively. I support adding the problem matchers back to improve the user experience. |
This reverts PR #88 (commit 00cae50).
The problem matchers are working as intended, I think.