Skip to content

[ci] Deprecate flake8#8409

Merged
mistercrunch merged 2 commits intoapache:masterfrom
john-bodley:john-bodley--deprecate-flake8
Oct 18, 2019
Merged

[ci] Deprecate flake8#8409
mistercrunch merged 2 commits intoapache:masterfrom
john-bodley:john-bodley--deprecate-flake8

Conversation

@john-bodley
Copy link
Member

@john-bodley john-bodley commented Oct 17, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

This PR deprecates flake8 (where we were using flake8-import-order and flake8-mypy) in favor of using isort and mypy directly. Note pylint (which we use though in many cases have warnings disabled) can be viewed as a superset flake8.

The reasons for the changes are:

  1. We've noticed in other repos that flake8-mypy has missed some issues that mypy discovers (fixed in this PR) and ignores some of the mypy settings.
  2. We were using flake8 mostly for checking import order and typing now that we're using black. Note that isort both checks and formats.

Also we were using flake8 for some other checks but simply ignoring these via # noqa which lacked specificity, i.e., reading the code it's not apparent what we're ignoring. Ideally we should be upping our pylint game as these checks would be caught by pylint. Currently many files have pylint warnings ignored and we have disabled a number of checks in the .pylintrc file per here. In the future we enforce pylint for all errors and warning and enable most (if not all) checks.

There are a few cases where I used # type: ignore as I ended up going down a rabbit hole when trying to resolve types. I think in the future we should try to re-enable these checks.

TEST PLAN

CI.

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

REVIEWERS

to: @betodealmeida @etr2460 @michellethomas @mistercrunch @villebro

@john-bodley john-bodley force-pushed the john-bodley--deprecate-flake8 branch from afe2229 to 03f2d3c Compare October 17, 2019 23:42
@codecov-io
Copy link

codecov-io commented Oct 18, 2019

Codecov Report

Merging #8409 into master will decrease coverage by 0.04%.
The diff coverage is 90.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8409      +/-   ##
==========================================
- Coverage   67.65%   67.61%   -0.05%     
==========================================
  Files         448      448              
  Lines       22506    22482      -24     
  Branches     2364     2364              
==========================================
- Hits        15227    15201      -26     
- Misses       7141     7143       +2     
  Partials      138      138
Impacted Files Coverage Δ
superset/models/sql_types/presto_sql_types.py 72.41% <ø> (ø) ⬆️
superset/views/datasource.py 95.12% <ø> (ø) ⬆️
superset/examples/tabbed_dashboard.py 22.22% <ø> (ø) ⬆️
superset/examples/multiformat_time_series.py 15.38% <ø> (ø) ⬆️
superset/examples/birth_names.py 100% <ø> (ø) ⬆️
superset/examples/bart_lines.py 29.62% <ø> (ø) ⬆️
superset/examples/world_bank.py 100% <ø> (ø) ⬆️
superset/examples/sf_population_polygons.py 29.16% <ø> (ø) ⬆️
superset/examples/deck.py 11.11% <ø> (ø) ⬆️
superset/utils/dict_import_export.py 21.05% <ø> (ø) ⬆️
... and 71 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 a199901...03f2d3c. Read the comment docs.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Nice work! LGTM, looks very clean. Just a few comments, mostly out of scope-stuff. @mistercrunch I propose including this in the 0.35 release, as these changes will most likely cause widespread conflicts when picking cherries for upcoming patch releases on the 0.35 branch.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM. I will open a separate PR to address the out of scope comments.

@mistercrunch
Copy link
Member

Confirmed with I can merge this, we're laying more refactors on top of this.

@mistercrunch mistercrunch merged commit 9fc37ea into apache:master Oct 18, 2019
graceguo-supercat pushed a commit that referenced this pull request Nov 13, 2019
* [ci] Deprecate flake8

* Addressing @villebro's comments
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.35.0 First shipped in 0.35.0 labels Feb 28, 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 size/XXL 🚢 0.35.0 First shipped in 0.35.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments