-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Eslint dependencies versions updates #10839
Conversation
b47deab
to
d8fafec
Compare
Hey @rusackas, can you please take a look? |
2a176dc
to
d8fafec
Compare
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! Thanks!
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.
a few questions, thanks for the PR
superset-frontend/src/explore/components/controls/SelectAsyncControl.jsx
Show resolved
Hide resolved
Also, maybe you could add comments next to the rules you disabled for this PR, that way we know which ones we want to reenable sooner rather than later |
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.
the zlib & path packages indicate a bit of a problem... if ESLint is throwing an error about these two, try resetting the pagkage-lock.json file to what's on master
, and it may shut up.
d8fafec
to
b8e4011
Compare
Codecov Report
@@ Coverage Diff @@
## master #10839 +/- ##
==========================================
- Coverage 61.34% 60.69% -0.66%
==========================================
Files 380 380
Lines 24068 24057 -11
==========================================
- Hits 14765 14601 -164
- Misses 9303 9456 +153
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@etr2460 I will add comments to the rules that will soon be re-enabled (I have a series of PRs incoming as soon as we merge this one 🙂). However, some of the rules require some discussion about whether we want to re-enable them or not. I'll mark them appropriately. |
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! Thanks for the cleanup and all the various tweaks!
* Update eslint version to 7.8.1 * Give names to unnamed functions to fix lint errors * Update eslint-import-resolver-webpack * Update eslint-plugin-cypress * Add eslint-plugin-react-hooks * Update necessary peer dependencies for eslint-config-airbnb * Update eslint airbnb config and ts plugins * Remove "this" from functional component * Disable all rules that cause new errors * Fix linting errors in tests * Add licenses to .eslintrc files * Add path and zlib to package.json * Disable incompatible rule in eslint-plugin-cypress * Remove redundant config for typescript linting * Mark disabled rules with comments * Remove path and zlib from deps, disable import rule for webpack files
SUMMARY
Upgrade eslint packages: eslint, eslint-config-airbnb, eslint-import-resolver-webpack, eslint-plugin-cypress and their dependencies.
This is the first pull request of the series - as upgrading eslint-config-airbnb caused a lot of new errors, I disabled the offending rules and I will re-enable them in subsequent PRs with proper fixes.
TEST PLAN
Run eslint on source files, verify that there are no new Javascript/Typescript errors.
ADDITIONAL INFORMATION