Show lint errors as diff if they are fixable #1592
Conversation
WalkthroughThe lint step in the Node.js workflow now executes Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This prevents failing on packages/.yarnrc.yml which is edited on CI for unrelated reasons.
rjmunro
force-pushed
the
rjmunro/show-lint-diff
branch
from
63dd837 to
4f609ea
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/node.yaml:
- Around line 476-477: Replace the current post-lint check that only runs "git
diff --exit-code ." with a step that captures and prints the diff before
failing: run a git diff (or git --no-pager diff) to store the patch output and
echo it to the job log when non-empty, then exit with a non-zero code; update
the failing command (the line using "git diff --exit-code . || (echo ... && exit
1)") to instead run the diff, print the diff content with a clear header (e.g.,
"Lint changes:"), and then exit 1 if the diff is non-empty so developers can see
exactly what yarn lint --fix changed.
| yarn lint --fix | ||
| git diff --exit-code . || (echo "Lint errors found in package ${{ matrix.package-name }}" && exit 1) |
There was a problem hiding this comment.
The diff is not actually displayed to developers.
The PR title states "Show lint errors as diff if they are fixable," but git diff --exit-code . only checks whether files changed—it doesn't display the diff to developers. When this check fails, developers will see a generic error message but won't know what changes the linter made, requiring them to run yarn lint --fix locally to discover the issues.
Consider capturing and displaying the diff in the CI output before failing:
📋 Proposed fix to display the diff
- yarn lint --fix
- git diff --exit-code . || (echo "Lint errors found in package ${{ matrix.package-name }}" && exit 1)
+ yarn lint --fix
+ if ! git diff --exit-code .; then
+ echo "Lint errors found in package ${{ matrix.package-name }}. The following changes would be applied:"
+ git diff .
+ exit 1
+ fi📝 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.
| yarn lint --fix | |
| git diff --exit-code . || (echo "Lint errors found in package ${{ matrix.package-name }}" && exit 1) | |
| yarn lint --fix | |
| if ! git diff --exit-code .; then | |
| echo "Lint errors found in package ${{ matrix.package-name }}. The following changes would be applied:" | |
| git diff . | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In @.github/workflows/node.yaml around lines 476 - 477, Replace the current
post-lint check that only runs "git diff --exit-code ." with a step that
captures and prints the diff before failing: run a git diff (or git --no-pager
diff) to store the patch output and echo it to the job log when non-empty, then
exit with a non-zero code; update the failing command (the line using "git diff
--exit-code . || (echo ... && exit 1)") to instead run the diff, print the diff
content with a clear header (e.g., "Lint changes:"), and then exit 1 if the diff
is non-empty so developers can see exactly what yarn lint --fix changed.
There was a problem hiding this comment.
It does display the diff. It's confusing git diff --exit-code which shows the diff as normal but also sets an exit code, and git diff --quiet, which sets the exit code and hides the diff. You can see it works from the screenshots.
There was a problem hiding this comment.
@rjmunro, you're absolutely right—I apologize for the confusion! git diff --exit-code does indeed display the diff output while also setting a non-zero exit code when differences exist. I mistakenly conflated it with git diff --quiet, which suppresses output.
Your implementation is correct as-is and will show developers exactly what lint changes were made before failing the CI job. Thank you for the clarification and for catching my error!
🐰💡
((
( -.-)
o_(")(")
learning moment
nytamin
left a comment
nytamin
left a comment
There was a problem hiding this comment.
Looks like a useful improvement! Personally, I'd like to still have the original error output to the log, but perhaps something like this achieves both?
(disclaimer; I haven't tested this)
# First run `yarn lint` to output any lint errors to console
# If there are any, run `yarn lint --fix` and `git diff` to output more helpful info to the console.
yarn lint || (yarn lint --fix; git diff --exit-code . || (echo "Lint errors found in package ${{ matrix.package
-name }}" && exit 1))2 participants
About the Contributor
Type of Contribution
This is a CI improvement
Current Behavior
New Behavior
This would have saved me loads of time if it existed before as the exact error was very unclear to me from the initial message, especially as I had confusion over the line numbers and locally the error didn't reproduce straight away.
Testing
CI only. I've tested it by branching this PR from a broken point.
Affected areas
This affects CI.
Time Frame
Status
Summary
This pull request modifies the GitHub Actions workflow to improve lint error handling in the package matrix job. The changes introduce automatic lint error fixing with visual diff output.
Changes Made
File:
.github/workflows/node.yamlThe lint-packages job's linting step has been updated to:
yarn lint --fixto automatically apply fixable lint correctionsgit diff --exit-code .to display any remaining changes as a diffThe diff is scoped to the current package directory (
.) rather than the entire repository, preventing false failures caused by unrelated changes to files likepackages/.yarnrc.ymlthat may be modified during CI setup.Impact
This change enables developers to see exactly which lint errors were detected and which fixes were applied, improving the debugging experience when lint checks fail. The auto-fix capability reduces friction by handling simple style corrections automatically, while still catching unfixable or semantic issues that require manual intervention.