Skip to content

chore(pre-commit): Add pyupgrade and pycln hooks #24197

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.

Sign up for GitHub

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

Jump to bottom
Merged
john-bodley merged 1 commit into from
Jun 1, 2023
Merged

chore(pre-commit): Add pyupgrade and pycln hooks #24197

john-bodley merged 1 commit into from
Jun 1, 2023

Conversation

@john-bodley
Copy link
Member

@john-bodley john-bodley commented May 24, 2023 •
edited
Loading

SUMMARY

I thought there was merit in "keeping up with the Joneses" in terms of ensuring our Python code is inline with the evolution of the language.

pyupgrade is a tool (and pre-commit hook) to automatically upgrade syntax for newer versions of the language. This PR adds said pre-commit hook (in addition to pycln which removes unused imports) to help ensure your code style format is fresh.

Apologies for the scale of the change. I wasn't able to find any setting in pyupgrade to make this more digestible, unless we opted to incrementally set the minimum Python requirement.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@john-bodley john-bodley force-pushed the john-bodley--pre-commit-hook-pyupgrade branch from 3573145 to e92b2d3 Compare May 24, 2023 08:02
github-advanced-security[bot]
github-advanced-security bot found potential problems May 24, 2023
View reviewed changes
superset/views/core.py Outdated

Check warning

Code scanning / CodeQL

URL redirection from remote source

Untrusted URL redirection depends on a [user-provided value](1).
@john-bodley john-bodley changed the title feature(pre-commit): Keeping up with the Joneses chore(python): Keeping up with the Joneses May 24, 2023
@john-bodley john-bodley changed the title chore(python): Keeping up with the Joneses chore: Keeping up with the Joneses - Python syntax May 24, 2023
@john-bodley john-bodley changed the title chore: Keeping up with the Joneses - Python syntax chore(python): Keeping up with the Joneses May 24, 2023
@john-bodley john-bodley force-pushed the john-bodley--pre-commit-hook-pyupgrade branch from 2328dc8 to e92b2d3 Compare May 24, 2023 16:39
@john-bodley john-bodley changed the title chore(python): Keeping up with the Joneses chore(pyupgrade): Keeping up with the Joneses May 24, 2023
john-bodley
john-bodley commented May 24, 2023
View reviewed changes
.pre-commit-config.yaml Outdated
Copy link
Member Author

@john-bodley john-bodley May 24, 2023

Choose a reason for hiding this comment

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

pyupgrade may perform type rewrites, etc. rendering some imports obsolete. It seemed prudent to also include a hook which then cleans up unused imports.

Copy link
Member

@michael-s-molina michael-s-molina May 31, 2023

Choose a reason for hiding this comment

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

Does pyupgrade is restrained to our supported Python versions? Another way of asking is if a new Python version is launched with a new syntax, will pyupgrade try to change the files to the new syntax before we support this new version?

Copy link
Member Author

@john-bodley john-bodley Jun 1, 2023

Choose a reason for hiding this comment

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

@michael-s-molina yes. Per the --py39-plus CLI argument it ensures that the changes are compatible with Python versions >= 3.9 which is the minimum version we currently support.

@john-bodley john-bodley marked this pull request as ready for review May 24, 2023 17:23
@john-bodley john-bodley requested a review from a team as a code owner May 24, 2023 17:23
@codecov
Copy link

codecov bot commented May 24, 2023 •
edited
Loading

Codecov Report

Merging #24197 (e7a2fe1) into master (f6e769a) will increase coverage by 0.00%.
The diff coverage is 93.71%.

❗ Current head e7a2fe1 differs from pull request most recent head b1cda7c. Consider uploading reports for the commit b1cda7c to get more accurate results

@@           Coverage Diff           @@
##           master   #24197   +/-   ##
=======================================
  Coverage   68.31%   68.31%           
=======================================
  Files        1957     1957           
  Lines       75597    75621   +24     
  Branches     8222     8222           
=======================================
+ Hits        51641    51661   +20     
- Misses      21848    21852    +4     
  Partials     2108     2108
Flag Coverage Δ
hive 53.39% <81.21%> (+0.02%) ⬆️
mysql 78.92% <92.81%> (+<0.01%) ⬆️
postgres 79.00% <92.88%> (+<0.01%) ⬆️
presto 53.32% <81.21%> (+0.02%) ⬆️
python 82.82% <93.71%> (+<0.01%) ⬆️
sqlite 77.53% <92.29%> (+<0.01%) ⬆️
unit 53.44% <80.38%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
superset/cli/main.py 0.00% <0.00%> (ø)
superset/cli/native_filters.py 0.00% <0.00%> (ø)
superset/commands/importers/v1/examples.py 0.00% <0.00%> (ø)
superset/common/tags.py 0.00% <0.00%> (ø)
superset/connectors/sqla/views.py 89.71% <0.00%> (ø)
superset/examples/bart_lines.py 0.00% <0.00%> (ø)
superset/examples/big_data.py 0.00% <0.00%> (ø)
superset/examples/countries.py 0.00% <0.00%> (ø)
superset/examples/multiformat_time_series.py 0.00% <0.00%> (ø)
superset/examples/paris.py 0.00% <0.00%> (ø)
... and 260 more

... and 11 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@john-bodley john-bodley force-pushed the john-bodley--pre-commit-hook-pyupgrade branch from e92b2d3 to 2af3468 Compare May 24, 2023 17:54
john-bodley
john-bodley commented May 24, 2023
View reviewed changes
.pre-commit-config.yaml Outdated
Copy link
Member Author

@john-bodley john-bodley May 24, 2023

Choose a reason for hiding this comment

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

pycln chokes on module not found errors when we try importing the default configs.

@john-bodley john-bodley changed the title chore(pyupgrade): Keeping up with the Joneses chore(pre-comit): Add pyupgrade and pycln hooks May 24, 2023
@john-bodley john-bodley changed the title chore(pre-comit): Add pyupgrade and pycln hooks chore(pre-commit): Add pyupgrade and pycln hooks May 24, 2023
@john-bodley john-bodley force-pushed the john-bodley--pre-commit-hook-pyupgrade branch 2 times, most recently from cfab874 to 8952492 Compare May 25, 2023 18:26
john-bodley
john-bodley commented May 25, 2023
View reviewed changes
superset/connectors/base/models.py Outdated
Copy link
Member Author

@john-bodley john-bodley May 25, 2023

Choose a reason for hiding this comment

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

See .python/mypy#15047 (comment).

@john-bodley john-bodley force-pushed the john-bodley--pre-commit-hook-pyupgrade branch 2 times, most recently from b8b64fa to 9e99d8c Compare May 25, 2023 19:02
michael-s-molina
michael-s-molina approved these changes Jun 1, 2023
View reviewed changes
Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you very much for this improvement @john-bodley! It will contribute a lot to code standardization.

Reviewing this PR made me aware of how much untyped structures (dict) we have in the codebase and the consequences of that in terms of bugs and duplicated logic 😟

I'm also going to ping @villebro @eschutho @jfrag1 in case they have extensions that may be affected by this PR.

superset/connectors/sqla/models.py Outdated
Copy link
Member

@michael-s-molina michael-s-molina Jun 1, 2023

Choose a reason for hiding this comment

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

It's way more readable then the Optional version 😄

Copy link
Member Author

@john-bodley john-bodley Jun 1, 2023

Choose a reason for hiding this comment

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

Yup. The term Optional is such a misnomer and a bad choice to represent something that could be None. The PEP-604 typing rewrites currently only apply to those files which contain the following,

from __future__ import annotations

though will become ubiquitous after we drop support for Python 3.9.

@john-bodley john-bodley force-pushed the john-bodley--pre-commit-hook-pyupgrade branch from 9e99d8c to b1cda7c Compare June 1, 2023 18:21
@john-bodley john-bodley merged commit a4d5d7c into apache:master Jun 1, 2023
@john-bodley john-bodley deleted the john-bodley--pre-commit-hook-pyupgrade branch June 1, 2023 19:01
@sfirke sfirke mentioned this pull request Sep 6, 2023
Merged
9 tasks
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 First shipped in 3.0.0 labels Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michael-s-molina michael-s-molina michael-s-molina approved these changes

@betodealmeida betodealmeida Awaiting requested review from betodealmeida betodealmeida is a code owner

@eschutho eschutho Awaiting requested review from eschutho eschutho is a code owner

@villebro villebro Awaiting requested review from villebro villebro is a code owner

Assignees

No one assigned

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XL 🚢 3.0.0 First shipped in 3.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@john-bodley @michael-s-molina @mistercrunch