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

Python 3.13 support #2306

Merged
merged 12 commits into from
Jan 15, 2025
Merged

Python 3.13 support #2306

merged 12 commits into from
Jan 15, 2025

Conversation

mayty
Copy link
Contributor

@mayty mayty commented Nov 22, 2024

Related issue: 2305

@mayty mayty marked this pull request as ready for review November 22, 2024 13:02
@king-phyte
Copy link

Since 3.8 is EOL, shouldn't we drop support for that too?

@mayty
Copy link
Contributor Author

mayty commented Dec 9, 2024

I think that Python 3.8 doesn't conflict with adding Python 3.13 support and dropping it is outside of this PR's scope

@king-phyte
Copy link

Fair point :)

@@ -14,7 +14,9 @@
"argparse",
"array",
"ast",
"asynchat",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these.

They were all removed in 3.12:

https://docs.python.org/3/whatsnew/3.12.html#asynchat-and-asyncore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove it, but it was added by the scripts. So it will automatically appear on the next run because it's still present in the corresponding documentation: https://docs.python.org/3.12/library/asynchat.html#module-asynchat

Mb it would be better to use stdlibs package or adopt it's approach to modules generation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha, the problem was caused by the docs re-adding module pages which had bee removed, but as stubs:

python/cpython#126622

Mb it would be better to use stdlibs package or adopt it's approach to modules generation?

Yeah, sounds like a good idea to use stdlibs 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the script to use stdlibs and ran it

"argparse",
"array",
"ast",
"asynchat",
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, this list contains lots of modules that have been removed in 3.12 or earlier.

Was this list created using the mkstdlibs.py script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all changes to stdlibbs/py*.py were generated by mkstdlibs.py scripts

Copy link
Contributor

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thank you!

@DanielNoord
Copy link
Member

Awesome! Could you rebase this so we can get this merged?

@mayty
Copy link
Contributor Author

mayty commented Jan 9, 2025

I had to drop python3.8 support to make builds pass on python3.13 because some packages required a version, which do not support python3.8 (not sure how it passed before the make actions happy PR merge)

At the same time, the prod package could have been installed with both 3.8 and 3.13, so I'm open to rolling back this commit if 3.8 support is considered more important than builds with python3.13 in PRs

@hugovk
Copy link
Contributor

hugovk commented Jan 9, 2025

I think it's fine to drop 3.8, as mentioned before, it's already EOL:

We could merge this as-is and do a more thorough 3.8 cleanup in another PR.

@DanielNoord
Copy link
Member

@mayty This can be rebased on main again :) Sorry for the churn! Just taking things slow.

@DanielNoord
Copy link
Member

See PyCQA/meta#64 (comment) I have stopped merging anything.

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.12%. Comparing base (26cf27d) to head (87d592e).
Report is 13 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2306   +/-   ##
=======================================
  Coverage   99.12%   99.12%           
=======================================
  Files          39       40    +1     
  Lines        3095     3096    +1     
  Branches      787      787           
=======================================
+ Hits         3068     3069    +1     
  Misses         15       15           
  Partials       12       12           

Copy link
Collaborator

@staticdev staticdev left a comment

Choose a reason for hiding this comment

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

@mayty thanks a lot for your contribution. Your changes look good to me. I kindly ask to rebase again and generate a new poetry.lock. The integration tests were failing and I just merged #2329.

Also, since you are supporting a higher version on Python, I also suggest to increase the integration tests version up to 3.12 on .github/workflows/integration.yml.

Copy link
Member

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

@staticdev Pushed the changes you requested to help move this along :)

There are two more remaining questions from my side. Other than that I think also think this looks good.

poetry.lock Show resolved Hide resolved
pyproject.toml Outdated
@@ -80,6 +81,7 @@ types-setuptools = ">=70.0.0.20240523"
types-colorama = ">=0.4.2"
types-toml = ">=0.1.3"
vulture = ">=1.0"
stdlibs = ">=2024.10.21.16"
Copy link
Member

Choose a reason for hiding this comment

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

It seems as if [tool.poetry.dev-dependencies] is almost in alphabetical order. Not sure if we want to add a commit to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm all for alphabetical order, but only if there is gonna be a commitment to maintaining it in the future (at least a mention in the coding-standart doc)

Copy link
Member

Choose a reason for hiding this comment

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

Up to @staticdev

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for alhabetical order.. I normally have in all project I maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorted dev dependencies

Copy link
Member

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Awesome!

Copy link
Collaborator

@staticdev staticdev left a comment

Choose a reason for hiding this comment

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

LGTM.

@staticdev staticdev merged commit 724d5e0 into PyCQA:main Jan 15, 2025
21 checks passed
@staticdev
Copy link
Collaborator

Thanks @mayty @DanielNoord @hugovk

@DanielNoord DanielNoord mentioned this pull request Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants