Skip to content
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

Merged
merged 16 commits into from
Sep 14, 2020
Merged

Conversation

kgabryje
Copy link
Member

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

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@kgabryje kgabryje changed the title [WIP] Eslint dependencies versions updates Eslint dependencies versions updates Sep 11, 2020
@kgabryje
Copy link
Member Author

Hey @rusackas, can you please take a look?

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

Copy link
Member

@etr2460 etr2460 left a 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/.eslintrc.js Outdated Show resolved Hide resolved
superset-frontend/.eslintrc.js Show resolved Hide resolved
superset-frontend/package.json Outdated Show resolved Hide resolved
superset-frontend/package.json Outdated Show resolved Hide resolved
@etr2460
Copy link
Member

etr2460 commented Sep 11, 2020

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

Copy link
Member

@rusackas rusackas left a 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.

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2020

Codecov Report

Merging #10839 into master will decrease coverage by 0.65%.
The diff coverage is n/a.

Impacted file tree graph

@@            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     
Flag Coverage Δ
#python 60.69% <ø> (-0.66%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/db_engines/hive.py 0.00% <0.00%> (-85.72%) ⬇️
superset/db_engine_specs/hive.py 53.90% <0.00%> (-30.08%) ⬇️
superset/db_engine_specs/presto.py 70.85% <0.00%> (-11.44%) ⬇️
superset/examples/world_bank.py 97.10% <0.00%> (-2.90%) ⬇️
superset/examples/birth_names.py 97.36% <0.00%> (-2.64%) ⬇️
superset/views/database/mixins.py 80.70% <0.00%> (-1.76%) ⬇️
superset/models/core.py 87.50% <0.00%> (-0.84%) ⬇️
superset/connectors/sqla/models.py 89.60% <0.00%> (-0.16%) ⬇️
superset/sql_parse.py 99.30% <0.00%> (-0.01%) ⬇️
superset/errors.py 100.00% <0.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 686c19a...f1eede2. Read the comment docs.

@kgabryje
Copy link
Member Author

@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.
@rusackas That error from ESLint seems legit as we do import zlib and path in webpack config files explicitly. If we don't want them to be listed in package.json deps, then maybe we should just disable the no-extraneous-dependencies rule?

@rusackas rusackas self-requested a review September 14, 2020 06:58
Copy link
Member

@rusackas rusackas left a 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!

@rusackas rusackas merged commit 8a774d5 into apache:master Sep 14, 2020
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* 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
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants