Fix urls entries in pyproject.toml with variable cases #803
Conversation
Reviewer's Guide by SourceryThis pull request fixes an issue where Sequence diagram for URL handling in Metadata.from_packagesequenceDiagram
participant P as Package
participant M as Metadata.from_package
P->>M: package.urls
Note over M: Convert URL key to lowercase
alt homepage URL
M->>M: Check if homepage matches
else repository URL
M->>M: Check if repository matches
else documentation URL
M->>M: Check if documentation matches
else other URLs
M->>M: Add to project_urls
end
Class diagram showing Package class URL handling changesclassDiagram
class Package {
-dict[str, str] _urls
+urls() dict[str, str]
+homepage str
}
note for Package "Added _urls field for testing
Modified urls property to handle
case-insensitive URL keys"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
for more information, see https://pre-commit.ci
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @gantoine - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/poetry/core/masonry/metadata.py
Outdated
| continue | ||
| if name == "repository" and url == package.urls["Repository"]: | ||
| if name_lower == "repository" and url == package.urls.get(name): |
There was a problem hiding this comment.
is this not always definitely true? because name and url are a pair from package.urls.items().
I am not sure what the intent of this code was but this variation seems pointless?
There was a problem hiding this comment.
perhaps the original code was just a roundabout way of reading package.repository_url - and doing that directly is a better fix?
There was a problem hiding this comment.
yeah i think the original code wasn't well written, i'll simplify it 👍🏼
src/poetry/core/packages/package.py
Outdated
| @@ -117,6 +117,9 @@ def __init__( | |||
|
|
|||
| self._yanked = yanked | |||
|
|
|||
| # Currently only used for testing purposes | |||
There was a problem hiding this comment.
this is not true, per the change at line 360.
either way I do not love this, why not set up homepage, repository url, and documentation url in testing?
There was a problem hiding this comment.
Sorry I should have clarified, this is happening in CI for a real project of mine, not just in testing. The addition of self._urls is for the tests i wrote in test_metadata.py, since url is a @property and can't be set directly.
There was a problem hiding this comment.
I would like to avoid adding production code that is only required for testing. Can you use a PropertyMock instead? See https://github.com/python-poetry/poetry/blob/5d8f8800b2cbb53da8057965663844bd1e04e455/tests/console/commands/test_check.py#L82-L86 for example.
There was a problem hiding this comment.
I think this is entirely redundant in the current version, the tests can manipulate the homepage and repository-url fields directly
This has been long time coming (for example for performance reasons), but was triggered by this poetry update that crashed the CI pipeline: - https://github.com/owasp-sbot/OSBot-Utils/actions/runs/12632720785/job/35198284338 - python-poetry/poetry#9961 - python-poetry/poetry#9957 - python-poetry/poetry-core#803
src/poetry/core/packages/package.py
Outdated
| @@ -117,6 +117,9 @@ def __init__( | |||
|
|
|||
| self._yanked = yanked | |||
|
|
|||
| # Currently only used for testing purposes | |||
There was a problem hiding this comment.
I would like to avoid adding production code that is only required for testing. Can you use a PropertyMock instead? See https://github.com/python-poetry/poetry/blob/5d8f8800b2cbb53da8057965663844bd1e04e455/tests/console/commands/test_check.py#L82-L86 for example.
src/poetry/core/masonry/metadata.py
Outdated
| if name == "repository" and url == package.urls["Repository"]: | ||
| continue | ||
| if name == "documentation" and url == package.urls["Documentation"]: | ||
| if name_lower == "repository" or name_lower == "documentation": |
There was a problem hiding this comment.
I guess the tests have started failing because you have changed the logic
probably want to check "if name is repository and url is repository-url", etc
|
Right updated the PR based on feedback, however it seems the downstream poetry tests are still failing with these changes. I assume I'll need to update those before this PR is accepted? |
|
poetry-core tests are also currently broken In passing I spotted PEP753 which says that poetry should change its handling of these anyway, so I have submitted #807 |
Otherwise, pip install . is currently not possible: python-poetry/poetry-core#803
Otherwise, pip install . is currently not possible: python-poetry/poetry-core#803
|
Superseded by #807 |
Otherwise, pip install . is currently not possible: python-poetry/poetry-core#803
Came across an issue trying to
poetry installtoday withstreaming-form-datain mypyproject.toml. The library's pyproject.toml uses lowercase keys for entries under[tool.poetry.urls], which breaks whenMetadata.from_packagetries to buildproject_urls. Added a couple tests for upper, lower and mixed-case entries.Summary by Sourcery
Tests:
Error logs from
docker build