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

'find_module' removed in Py3.12; use 'find_spec' instead #1320

Merged
merged 5 commits into from
Apr 10, 2024
Merged

Conversation

javulticat
Copy link
Member

Description

Shout out to @dennybiasiolli for pointing out this issue in #1311! 🙏 However, instead of the branching logic that creates separate codepaths for Py3.12 found in that PR, I believe a simpler solution is possible:

Since the find_spec impl is backwards compatible with find_module and has been available since Py3.4, it should work as a drop-in replacement on all of the Python runtimes Zappa currently supports (Py3.8 - Py3.12), so branching logic shouldn't be necessary.

Note: I intentionally kept this PR as small as possible in order to help expedite it, as it's one of the last remaining blockers for our next release that will bring long-awaited support for Py3.12 to Zappa. That said, further improvements can definitely be made to Zappa's aging CLI impl, such as following DRY principles to consolidate the repetitive lines of code seen here. However, IMO, code cleanup is low priority such that it shouldn't block our next release; instead, it can be considered for inclusion during planning for subsequent Zappa release cycles.

Related

Supersedes #1311
Relates to #1291

@javulticat javulticat added python Related to Python support needs-review Needs attention from a maintainer priority | P2 Priority: Medium labels Apr 3, 2024
@javulticat javulticat added this to the Zappa 0.59 milestone Apr 3, 2024
@javulticat javulticat requested review from hellno and monkut April 3, 2024 13:26
@javulticat javulticat self-assigned this Apr 3, 2024
@javulticat javulticat enabled auto-merge (squash) April 3, 2024 13:27
@coveralls
Copy link

coveralls commented Apr 3, 2024

Coverage Status

coverage: 74.668% (-0.2%) from 74.831%
when pulling cb0ac96 on jav/py3.12
into 4b4ba4c on master.

Copy link
Contributor

@dennybiasiolli dennybiasiolli left a comment

Choose a reason for hiding this comment

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

Nice! And without an "if" in the code, makes perfect sense 😉

@javulticat javulticat added priority | P1 Priority: High and removed priority | P2 Priority: Medium labels Apr 7, 2024
@javulticat javulticat disabled auto-merge April 10, 2024 17:37
@javulticat javulticat merged commit b272361 into master Apr 10, 2024
6 checks passed
@javulticat javulticat deleted the jav/py3.12 branch April 10, 2024 17:37
@javulticat javulticat removed the needs-review Needs attention from a maintainer label Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority | P1 Priority: High python Related to Python support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants