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

Update the code to support Python 3.12.4 #10

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

JimZhouZZY
Copy link
Contributor

@JimZhouZZY JimZhouZZY commented Jun 24, 2024

Changes:

  1. Replaced the use of the pkg_resources method with the newer importlib method. For detailed guidance on this, please refer to this page.

  2. Renamed the file types.py to type_definitions.py to prevent conflicts with the similarly named file in the Python library.

@JimZhouZZY
Copy link
Contributor Author

Could not pass the tests due to python version changes.

@Grimmys
Copy link
Owner

Grimmys commented Jun 25, 2024

It's good changes, for a second I thought we were in the RPG project and I was going to say it's maybe to early to do such change (since 3.11 is out since October 24, 2022 only), but it's the library repo so I think it's fine!

Just a few other places should be updated in order to make this upgrade:

  • setup.py to indicate Python 3.12 is required to build the project
  • .github/workflows/test-library.yml to change the version of Python used to run the tests, hopefully fixing them

@@ -8,7 +8,7 @@
from os.path import abspath
from typing import Union

import pkg_resources
import importlib.resources as res
Copy link
Owner

Choose a reason for hiding this comment

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

I don't really like extensive abbreviations, because they can be confusing sometimes.
I suggest to replace it by from importlib import resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

@JimZhouZZY
Copy link
Contributor Author

Changed import method .
Modified some other files to adapt the use of importlib.
Renamed import codes to adapt the new name type_definitions.py of the old types.py

@JimZhouZZY
Copy link
Contributor Author

JimZhouZZY commented Jun 26, 2024

It's good changes, for a second I thought we were in the RPG project and I was going to say it's maybe to early to do such change (since 3.11 is out since October 24, 2022 only), but it's the library repo so I think it's fine!

Just a few other places should be updated in order to make this upgrade:

  • setup.py to indicate Python 3.12 is required to build the project
  • .github/workflows/test-library.yml to change the version of Python used to run the tests, hopefully fixing them

Thank you for the help, I now successfully passed the unit test. However it failed to build docs. I've checked the error messages, seems like the __name__ here refers to pygamepopup.configuration itself, causing the error.

I would like to use __package__ instead of __name__ here to prevent this issue.

Additionaly, I changed the version number to 0.10.1 and updated CHANGELOG.txt. I'm still not sure about the version number. Please take a look and correct the version number and the changelog if they are not good.

@JimZhouZZY JimZhouZZY requested a review from Grimmys June 26, 2024 05:29
@Grimmys
Copy link
Owner

Grimmys commented Jul 5, 2024

The version should rather be a minor one than a patch one, because it's changing the way consumers should use the module (they cannot integrate it anymore in a project running on Python < 3.12).

I'm still thinking about this change because I didn't immediately realized it would not be backward compatible anymore... Meaning the minimal version of Python for the RPG project for example would have to be increased to 3.12. And 3.12 is a very recent version (less than a year) so not sure if it's fair to force users to be on this latest version?
At the mean time I guess it's good motivation for people to upgrade to this newest version.

@JimZhouZZY
Copy link
Contributor Author

Understood, I'll make changes on these version numbers. Let's leave this PR open and merge it later.

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.

2 participants