fix(linter): report correct diagnostic count on minified files#13896
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where the linter was not reporting the correct diagnostic count on minified files. The fix ensures that when a minified file is detected, subsequent diagnostics are properly skipped from output while still being counted.
- Introduces tracking of minified file state to prevent output of diagnostics after minification is detected
- Replaces
breakwithcontinueto maintain diagnostic counting while suppressing output - Adds condition to skip diagnostic output for minified files similar to silent mode
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
CodSpeed Instrumentation Performance ReportMerging #13896 will not alter performanceComparing Summary
Footnotes |
|
I'm not sure if this is correct or not. I can confirm that the number of warnings + errors reported in summary is now the same with / without But... the number in summary does not correspond to the actual number of warnings/errors output. In Kibana repo, after this PR: Previously: Is this intentional? (I'm unclear why we skip linting minified files anyway, I'd have expected user should exclude any files they don't want linted) |
|
tbh i think this is fine. It isn't perfect but it's happening because, we are printing a warning diagnostic, when we find a minified file, and skipping the rest of the diagnostics for this file. people should be ignoring stuff in their dist directories anyway so this is very much an edge case imo |
Merge activity
|
fixes #13865 testing this is tricky as we have a different impl for cfg test which means this code path is never hit. but i tested this in vscode repo and appears to work as expected
1f41408 to
2a7d5d3
Compare

fixes #13865
testing this is tricky as we have a different impl for cfg test which means this code path is never hit. but i tested this in vscode repo and appears to work as expected