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

Use importlib.resources to load schema and licenses #670

Merged

Conversation

edgarrmondragon
Copy link
Contributor

@edgarrmondragon edgarrmondragon commented Dec 2, 2023

Again, this is one step towards resolving python-poetry/poetry#2965.

  • Added tests for changed code.
  • Updated documentation for changed code.

Another attempt after #668 fell through.

@edgarrmondragon edgarrmondragon marked this pull request as ready for review December 2, 2023 01:39
Copy link
Contributor

@dimbleby dimbleby left a comment

Choose a reason for hiding this comment

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

my overall feeling is that this is a lot of code to be vendoring for a rather niche use case - poetry in a zipapp - which, presumably, has been unsupported for all this time anyway.

Maybe wait until python 3.8 goes eol next year and then come back to this?

@@ -22,6 +22,7 @@ classifiers = [
python = "^3.8"

fastjsonschema = "^2.18.0"
importlib-resources = { version = ">=1.3", python = "<3.9" }
Copy link
Contributor

Choose a reason for hiding this comment

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

<3.9 is not helpful here: importlib-resources has to be either vendored with poetry-core, or not.

This is a decision that is made at make vendor/update time: it is not desirable that maintainers and contributors using different python versions get different answers

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've removed the vendored code per #670 (comment)

@radoering
Copy link
Member

I'd like to avoid additional vendoring, too. An alternative to waiting for the EOL of Python 3.8 might be to keep the old way for Python 3.8 and only use importlib.resources for Python 3.9 and above.

@edgarrmondragon
Copy link
Contributor Author

I'd like to avoid additional vendoring, too. An alternative to waiting for the EOL of Python 3.8 might be to keep the old way for Python 3.8 and only use importlib.resources for Python 3.9 and above.

That makes sense to me 👍

* Removed `importlib_resources` and `zipp` vendored code
* Continue using `__file__` in Python 3.8
* Only use `importlib.resources` in Python >= 3.9
Copy link

sonarcloud bot commented Dec 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@edgarrmondragon
Copy link
Contributor Author

@radoering @dimbleby this is ready for another review 🙂

@radoering radoering merged commit dc8e254 into python-poetry:main Dec 9, 2023
20 checks passed
@edgarrmondragon edgarrmondragon deleted the 2965-importlib.resources branch January 13, 2024 00:24
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.

3 participants