[No QA] Remove unused eslint-disable directives#83798
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
303ce76 to
6ac4ea9
Compare
|
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
d177dd3 to
0f70f0f
Compare
Upgrade eslint-config-expensify to 2.0.106 which sets reportUnusedDisableDirectives: 'error', and auto-fix all stale directives across the codebase. Made-with: Cursor
Some rules (no-restricted-syntax, @typescript-eslint/no-deprecated, no-default-id-values, no-restricted-imports) are only active in the changed-files ESLint config. Restore the disable directives that were present on main and are still needed for those rules. Made-with: Cursor
d6fcf07 to
c4db6c4
Compare
|
@MarioExpensify Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
The rule that's failing in the main config needs to be disabled, one sec |
The shared eslint-config-expensify sets this to 'error', but the main config must override it to 'off' since some disable directives are only needed by the stricter eslint.changed.config.mjs rules. Also restore the no-empty disable comment in BaseAutoCompleteSuggestions. Made-with: Cursor
|
Just to be clear, because this file touches 300+ files, it's not my intention to fix all the lint-changed errors, because they are pre-existing and many of them are risky/non-trivial to fix |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
@roryabraham looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
| // If params are defined, but reportID is explicitly undefined, we will get the url /r/undefined. | ||
| // We want to avoid that situation, so we will return an empty string instead. | ||
| parse: { | ||
| // eslint-disable-next-line |
There was a problem hiding this comment.
Hi @roryabraham
This suppression removal causing ESLint fail here on PR #80020
| key: `${ONYXKEYS.COLLECTION.NEXT_STEP}${iou.report.reportID}`, | ||
| onyxMethod: Onyx.METHOD.SET, | ||
| // buildOptimisticNextStep is used in parallel | ||
| // eslint-disable-next-line @typescript-eslint/no-deprecated |
There was a problem hiding this comment.
This and other no-deprecated removals in this file caused a huge ESLint fail on this PR: https://github.com/Expensify/App/actions/runs/22720110148/job/65879696179?pr=84069
i.e.
buildNextStepNewis deprecated.runAfterInteractionsis deprecated.translateLocalis deprecated.getPolicyis deprecated.getPolicyTagsDatais deprecated.
The eslint-disable-next-line @typescript-eslint/no-deprecated comments were removed in a recent cleanup (#83798) but only partially restored. This restores the remaining 28 missing directives that are needed for the Changed files ESLint check to pass. Co-authored-by: DylanDylann <DylanDylann@users.noreply.github.com>
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.32-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.32-3 🚀
|
Explanation of Change
Upgrades
eslint-config-expensifyto a version that includesreportUnusedDisableDirectives: 'error'in the base config (defaults to warn), temporarily removed the local override ineslint.config.mjsthat previously disabled this check, and auto-fixes all ~1100 staleeslint-disabledirectives across the codebase.Fixed Issues
N/A - tooling improvement
Tests
npx eslint src/ tests/ .github/ scripts/and verify noUnused eslint-disable directiveerrors appearOffline tests
N/A - no runtime behavior change
QA Steps
N/A - no runtime behavior change. This is a lint-only change.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
N/A - no UI changes