Skip to content

root: Upgrade Python syntax for version 3.11 using pyupgrade#7092

Closed
PKizzle wants to merge 12 commits intogoauthentik:mainfrom
PKizzle:feature/upgrade-syntax-to-python-3.11
Closed

root: Upgrade Python syntax for version 3.11 using pyupgrade#7092
PKizzle wants to merge 12 commits intogoauthentik:mainfrom
PKizzle:feature/upgrade-syntax-to-python-3.11

Conversation

@PKizzle
Copy link
Contributor

@PKizzle PKizzle commented Oct 6, 2023 •
edited
Loading

Result of running pyupgrade --py311-plus `find ./authentik/ -name "*.py" -type f` as project uses Python 3.11

Also, consider adding the pyupgrade command to the GitHub actions to automatically upgrade all new code.
Be aware that the developer is unwilling to allow ignoring certain source code lines so it may break certain functionality that requires workarounds.
In this case several manual adjustments were necessary as there are type collisions and the new Element | None type hint instead of Optional[Element] does not work reliably.

More information can be found here: https://github.com/asottile/pyupgrade

@PKizzle PKizzle requested a review from a team as a code owner October 6, 2023 14:17
@netlify
Copy link

netlify bot commented Oct 6, 2023 •
edited
Loading

✅ Deploy Preview for authentik-storybook canceled.

Name Link
🔨 Latest commit da969b8
🔍 Latest deploy log https://app.netlify.com/sites/authentik-storybook/deploys/6549110b1b0c8600083980f9

@PKizzle PKizzle changed the title Initial commit. Upgrade Python syntax for version 3.11 using pyupgrade Oct 6, 2023
@PKizzle PKizzle changed the title Upgrade Python syntax for version 3.11 using pyupgrade root: Upgrade Python syntax for version 3.11 using pyupgrade Oct 6, 2023
@PKizzle PKizzle marked this pull request as draft October 6, 2023 14:31
@codecov
Copy link

codecov bot commented Oct 6, 2023 •
edited
Loading

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (fe1a06e) 90.27% compared to head (da969b8) 92.58%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7092      +/-   ##
==========================================
+ Coverage   90.27%   92.58%   +2.30%     
==========================================
  Files         587      587              
  Lines       28898    28858      -40     
==========================================
+ Hits        26088    26718     +630     
+ Misses       2810     2140     -670
Flag Coverage Δ
e2e 50.95% <90.25%> (+4.90%) ⬆️
integration 25.95% <67.17%> (?)
unit 89.58% <99.48%> (-0.02%) ⬇️

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

Files Coverage Δ
authentik/admin/api/tasks.py 100.00% <ø> (ø)
authentik/api/authentication.py 96.92% <100.00%> (ø)
authentik/api/decorators.py 100.00% <100.00%> (ø)
authentik/api/tests/test_auth.py 100.00% <100.00%> (ø)
authentik/api/tests/test_viewsets.py 100.00% <100.00%> (ø)
authentik/blueprints/api.py 85.93% <100.00%> (ø)
authentik/blueprints/models.py 96.96% <100.00%> (ø)
authentik/blueprints/tests/__init__.py 100.00% <100.00%> (ø)
authentik/blueprints/tests/test_packaged.py 95.65% <100.00%> (ø)
...thentik/blueprints/tests/test_serializer_models.py 100.00% <100.00%> (ø)
... and 147 more

... and 21 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@PKizzle PKizzle marked this pull request as ready for review October 6, 2023 15:27
BeryJu
BeryJu approved these changes Oct 6, 2023
View reviewed changes
Copy link
Member

@BeryJu BeryJu left a comment

Choose a reason for hiding this comment

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

LGTM, allthough we might want to wait for #7048 and then directly go to python 3.12

@PKizzle
Copy link
Contributor Author

PKizzle commented Oct 6, 2023 •
edited
Loading

Shall I open a new PR with pyupgrade running for Python 3.12?

Edit: I tried upgrading to Python 3.12 but had issues installing the necessary pip packages (multidict does not want to build)

@PKizzle PKizzle mentioned this pull request Oct 6, 2023
Closed
@PKizzle
Copy link
Contributor Author

PKizzle commented Oct 27, 2023 •
edited
Loading

@BeryJu I have updated the PR to incorporate the changes of #7096.

Seems like the upgrade to Python 3.12 might take some time. Is there any reason not to merge this PR for version 3.11?
Running pyupgrade for 3.12 only introduces one additional change compared to this PR.

What is missing in this PR is a way to automatically run pyupgrade but that shall be handled in another PR imho.

@PKizzle
Copy link
Contributor Author

PKizzle commented Oct 27, 2023 •
edited
Loading

authentik/stages/prompt/api.py also uses a "type" field which requires a pyupgrade workaround. I will create another issue to fix this as well. (This is the reason the CI is currently failing).

@PKizzle PKizzle mentioned this pull request Oct 28, 2023
Closed
@netlify
Copy link

netlify bot commented Nov 6, 2023 •
edited
Loading

✅ Deploy Preview for authentik ready!

Name Link
🔨 Latest commit dea4ac7
🔍 Latest deploy log https://app.netlify.com/sites/authentik/deploys/6549053ab0bd7d00081467cc
😎 Deploy Preview https://deploy-preview-7092--authentik.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse		 1 paths audited
Performance:
Accessibility:
Best Practices:
SEO:
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@PKizzle PKizzle force-pushed the feature/upgrade-syntax-to-python-3.11 branch from 027e128 to 7c3ba64 Compare November 6, 2023 15:44
@PKizzle PKizzle force-pushed the feature/upgrade-syntax-to-python-3.11 branch from 7c3ba64 to b8f51d9 Compare November 6, 2023 16:08
@BeryJu
Copy link
Member

BeryJu commented Feb 24, 2024

I think most of these changes were also made when we switched to ruff (#8498) which has pretty much the same policies and also allows disabling it for specific lines like the XML Element | None not working

@BeryJu
Copy link
Member

BeryJu commented Mar 15, 2024

As mentioned in the comment above this has already been addressed by our change to ruff and also we're targeting python 3.12 now, so we'll close this.

@BeryJu BeryJu closed this Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BeryJu BeryJu BeryJu approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PKizzle @BeryJu