-
-
Notifications
You must be signed in to change notification settings - Fork 207
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
Reorganize ESLint warning thresholds and output #5159
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
b83c052
to
83b44c2
Compare
4 tasks
Some lint rules are disabled because they generate many violations across the monorepo (mostly thanks to the recent ESLint upgrade). To be able to identify them and remove them, we have set these rules to generate warnings instead, and we have also implemented a quality gate to ensure that no new instances of these warnings occur in the codebase. However, currently there are so many warnings and the output from ESLint is so large, that if changes are made in a branch that do end up producing new violations, it is basically impossible to know where those violations occurred. The format of the warning thresholds file, which is used to track changes to warnings, does not help things, either, because it only records the rules for which violations occurred and not the files that have those violations. To address this, this commit reorganizes the warning thresholds file so that warning counts are filed first by file path and then by rule. A regression or improvement in lint violations is still determined by calculating a total and comparing it to the file, but then the script is able to show where new violations occurred. (As a plus, this commit also hides improvements if there are regressions.) For instance, instead of saying something like: ``` 🛑 New ESLint warnings have been introduced and need to be resolved for linting to pass: - @typescript-eslint/consistent-type-exports: 19 -> 21 (+2) - typescript-eslint/no-base-to-string: 3 -> 1 (-2) - typescript-eslint/no-duplicate-enum-values: 2 -> 3 (+1) ``` it might now say: ``` 🛑 New lint violations have been introduced and need to be resolved for linting to pass: - packages/controller-utils/src/types.ts - typescript-eslint/no-duplicate-enum-values: 2 -> 3 (+1) - packages/logging-controller/src/logTypes/index.ts - @typescript-eslint/consistent-type-exports: 0 -> 1 (+1) - packages/message-manager/src/index.ts - @typescript-eslint/consistent-type-exports: 0 -> 1 (+1) - packages/notification-services-controller/src/NotificationServicesController/index.ts - @typescript-eslint/consistent-type-exports: 0 -> 1 (+1) ```
da2cf50
to
f4580a5
Compare
cryptodev-2s
previously approved these changes
Feb 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
cryptodev-2s
approved these changes
Feb 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Some lint rules are disabled because they generate many violations across the monorepo (mostly thanks to the recent ESLint upgrade). To be able to identify them and remove them, we have set these rules to generate warnings instead, and we have also implemented a quality gate to ensure that no new instances of these warnings occur in the codebase.
However, currently there are so many warnings and the output from ESLint is so large, that if changes are made in a branch that do end up producing new violations, it is basically impossible to know where those violations occurred. The format of the warning thresholds file, which is used to track changes to warnings, does not help things, either, because it only records the rules for which violations occurred and not the files that have those violations.
To address this, this commit reorganizes the warning thresholds file so that warning counts are filed first by file path and then by rule. A regression or improvement in lint violations is still determined by calculating a total and comparing it to the file, but then the script is able to show where new violations occurred. (As a plus, this commit also hides improvements if there are regressions.)
For instance, instead of saying something like:
it might now say:
Manual testing
scripts/run-eslint.ts
)yarn lint:eslint
, observe that the file affected is now listed in the outputpackages/user-operation-controller/src/utils/validation.test.ts
)yarn lint:eslint
, observe that the file affected is now listed in the outputReferences
Prerequisite to #5187, which will help a lot.
Changelog
(N/A, developer-only change)
Checklist