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 json schemas & spdx version #668

Closed

Conversation

gpshead
Copy link

@gpshead gpshead commented Nov 28, 2023

This is part of the solution for python-poetry/poetry#2965.

Constructing a regression integration test that uses a zipapp like environment would be useful but is not part of this PR.

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

Currently unclear to me what additional testing is required to accept this. I'm making the PR because this change works for one use case for us within our custom bundled app environment and it passes this repo's tests/ and tests/integration suites. I'm curious to see what your Github CI runs turn up.

Does this need documentation beyond the changelog? It's an internal implementation detail change.

I have not audited the code looking for other resource loading code paths so some may exist, thus no claim that this actually completely fixes the related issue. It works for our own limited usage at work on 3.10+ and hasn't tickled and tripped over things yet. (in particular I see more in the main poetry repo, not just poetry-core such as poetry's src/json/__init__.py being similar to this repo's src/poetry/core/json/__init__.py - but that'd be another PR in the other repo).

gpshead and others added 2 commits November 28, 2023 12:46
This is part of the fix for
python-poetry/poetry#2965.

Constructing a regression integration test that uses a zipapp like
environment would be useful but is not part of this PR.
@gpshead
Copy link
Author

gpshead commented Nov 28, 2023

CI seems to be failing due to unrelated pre-existing static type analysis issues from mypy that exist on main:

src/poetry/core/version/pep440/parser.py:84: error: Returning Any from function declared to return "T"  [no-any-return]
src/poetry/core/version/pep440/parser.py:84: note: Error code "no-any-return" not covered by "type: ignore" comment
src/poetry/core/version/markers.py:540: error: Unused "type: ignore" comment  [unused-ignore]
src/poetry/core/version/markers.py:709: error: Unused "type: ignore" comment  [unused-ignore]

@dimbleby
Copy link
Contributor

the pipeline is failing because you have updated mypy - either fix the type annotations, or dont do that! (poetry lock --no-update)

having said which poetry-core vendors dependencies rather than requiring them to be downloaded when it is used as a build-backend - so probably that is what you actually should do

@gpshead
Copy link
Author

gpshead commented Nov 28, 2023

the pipeline is failing because you have updated mypy - either fix the type annotations, or dont do that! (poetry lock --no-update)

Technically accurate, but here's why: I'd expect any new contributor unfamiliar with poetry to his this problem. I found no instructions on how to build and test anything or update the lock file in this repo.

So I resorted to manually running commands seen in .github/workflows/tests.yml. The poetry lock --check step failed and said I should run poetry lock. I did. The "Error: poetry.lock is not consistent with pyproject.toml. Run 'poetry lock [--no-update]' to fix it." message produced by that doesn't tell me whether or not I should add --no-update. Without knowing anything about poetry lock files, --no-update as a flag name sounds like it would not update the file. So I did not try that because it didn't tell me I was required to. Only people already pretty familiar with poetry would have that knowledge.

If you write a contributors guide that describes things like this, it'd help anyone making future PRs.

@dimbleby
Copy link
Contributor

per my last, you don't actually want to add this dependency in that pyproject.toml at all, it wants to be vendored

the version-dependent import wants hiding away in utils/_compat.py (the main poetry repository has a similar file and does the same for importlib-metadata, copying that should be defensible)

Copy link

sonarcloud bot commented Nov 28, 2023

Please retry analysis of this Pull-Request directly on SonarCloud.

@gpshead
Copy link
Author

gpshead commented Nov 28, 2023

Okay, i've spend more time than this is worth attempting to make what I thought was a simple change. I'll leave this to someone familiar with the more complicated than anticipated internals of this project to take over and complete.

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