Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/poetry/core/masonry/metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,12 @@ def from_package(cls, package: ProjectPackage) -> Metadata:

if package.urls:
for name, url in package.urls.items():
if name.lower() == "homepage" and meta.home_page == url:
name_lower = name.lower()
if name_lower == "homepage" and meta.home_page == url:
continue
if name == "repository" and url == package.urls["Repository"]:
if name_lower == "repository" and url == package.urls.get(name):
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@dimbleby dimbleby Jan 6, 2025

Choose a reason for hiding this comment

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

perhaps the original code was just a roundabout way of reading package.repository_url - and doing that directly is a better fix?

Copy link
Author

Choose a reason for hiding this comment

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

yeah i think the original code wasn't well written, i'll simplify it 👍🏼

continue
if name == "documentation" and url == package.urls["Documentation"]:
if name_lower == "documentation" and url == package.urls.get(name):
continue

meta.project_urls += (f"{name}, {url}",)
Expand Down
6 changes: 6 additions & 0 deletions src/poetry/core/packages/package.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,9 @@ def __init__(

self._yanked = yanked

# Currently only used for testing purposes
Copy link
Contributor

@dimbleby dimbleby Jan 6, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

https://github.com/rommapp/romm/actions/runs/12623342037/job/35174039307#step:10:3085

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is entirely redundant in the current version, the tests can manipulate the homepage and repository-url fields directly

self._urls: dict[str, str] = {}

@property
def name(self) -> NormalizedName:
return self._name
Expand Down Expand Up @@ -355,6 +358,9 @@ def all_classifiers(self) -> list[str]:
def urls(self) -> dict[str, str]:
urls = {}

if self._urls:
return self._urls

if self.homepage:
urls["Homepage"] = self.homepage

Expand Down
51 changes: 51 additions & 0 deletions tests/masonry/test_metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,54 @@ def test_from_package_readme_issues(
Metadata.from_package(package)

assert str(e.value) == message


def test_from_package_urls_case_sensitive() -> None:
package = ProjectPackage("foo", "1.0")
package.homepage = "https://example.com"
package._urls = {
"Homepage": "https://example.com",
"Repository": "https://github.com/example/repo",
"Documentation": "https://docs.example.com",
"Other": "https://other.example.com",
}

metadata = Metadata.from_package(package)

# Only "Other" should be in project_urls since others are special cases
assert len(metadata.project_urls) == 1
assert metadata.project_urls[0] == "Other, https://other.example.com"


def test_from_package_urls_case_mixed() -> None:
package = ProjectPackage("foo", "1.0")
package.homepage = "https://example.com"
package._urls = {
"homepage": "https://example.com",
"Repository": "https://github.com/example/repo",
"DOCUMENTATION": "https://docs.example.com",
"other": "https://other.example.com",
}

metadata = Metadata.from_package(package)

# Only "other" should be in project_urls since others are special cases
assert len(metadata.project_urls) == 1
assert metadata.project_urls[0] == "other, https://other.example.com"


def test_from_package_urls_lowercase() -> None:
package = ProjectPackage("foo", "1.0")
package._urls = {
"homepage": "https://example.com",
"repository": "https://github.com/example/repo",
"documentation": "https://docs.example.com",
"other": "https://other.example.com",
}

metadata = Metadata.from_package(package)

# Only "other" should be in project_urls since others are special cases
assert len(metadata.project_urls) == 2
assert metadata.project_urls[0] == "homepage, https://example.com"
assert metadata.project_urls[1] == "other, https://other.example.com"
Loading