Skip to content

Comments

add source validation in poetry check#8709

Merged
radoering merged 1 commit intopython-poetry:masterfrom
lucemia:fix-8704-validate-source
Dec 11, 2023
Merged

add source validation in poetry check#8709
radoering merged 1 commit intopython-poetry:masterfrom
lucemia:fix-8704-validate-source

Conversation

@lucemia
Copy link
Contributor

@lucemia lucemia commented Nov 25, 2023 •
edited
Loading

Pull Request Check List

  • Resolves: poetry check does not consider the package source #8704
  • add two one types of source validation in poetry check
    • Invalid source "{source}" referenced in dependencies. which means a dependency referenced to not exist source
    • Dependency "{k}" referenced to inconsistence sources. which means a dependency referenced to more than one source
  • Added tests for changed code.
  • Updated documentation for changed code.

@radoering
Copy link
Member

radoering commented Dec 1, 2023

Checking for inconsistent sources is not that easy. For example, the following inconsistent sources are not detected:

inconsistent_sources = [
    { version = "*", source = "not-consistence-1" },
    { version = "*", source = "not-consistence-2" },
]

and even worse, the following consistent sources are detected as inconsistent (same for python or markers instead of platform):

[tool.poetry.dependencies]
consistent_sources = { source = "not-consistence-1", platform = "win32" }

[tool.poetry.group.test.dependencies]
consistent_sources = { source = "not-consistence-2", platform = "linux" }

radoering
radoering requested changes Dec 1, 2023
View reviewed changes
Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

Checking for not existing sources is probably ok, but checking for inconsistent sources can lead to false positives (see #8709 (comment)).

@lucemia
Copy link
Contributor Author

lucemia commented Dec 2, 2023

Thanks, @radoering. It's definitely more intricate than I initially anticipated. I'm going to delve deeper to explore potential solutions. Meanwhile, I would like to separate the pull request (PR) and create an independent one specifically for Checking for non-existing sources.

@lucemia lucemia force-pushed the fix-8704-validate-source branch 2 times, most recently from 03ec9db to 3b8c9ad Compare December 4, 2023 01:36
@lucemia lucemia requested a review from radoering December 4, 2023 01:46
@lucemia
Copy link
Contributor Author

lucemia commented Dec 4, 2023

@radoering, I've updated the pull request to focus on checking for sources that don't exist. While making changes, I realized that moving the logic to src/poetry/factory.py could be a viable alternative. I'm ok to proceed in either direction.

poetry/src/poetry/factory.py

Lines 357 to 379 in 0578ed8

def validate(
cls, config: dict[str, Any], strict: bool = False
) -> dict[str, list[str]]:
results = super().validate(config, strict)
results["errors"].extend(validate_object(config))
# A project should not depend on itself.
dependencies = set(config.get("dependencies", {}).keys())
dependencies.update(config.get("dev-dependencies", {}).keys())
groups = config.get("group", {}).values()
for group in groups:
dependencies.update(group.get("dependencies", {}).keys())
dependencies = {canonicalize_name(d) for d in dependencies}
project_name = config.get("name")
if project_name is not None and canonicalize_name(project_name) in dependencies:
results["errors"].append(
f"Project name ({project_name}) is same as one of its dependencies"
)
return results

radoering
radoering requested changes Dec 4, 2023
View reviewed changes
@radoering
Copy link
Member

radoering commented Dec 4, 2023 •
edited
Loading

While making changes, I realized that moving the logic to src/poetry/factory.py could be a viable alternative. I'm ok to proceed in either direction.

IMO, it's good as is. Moving it into the factory means that it is always executed (performance) and prevents any command. I think neither of these is necessary or wanted.

Edit: We could put it beyond strict to achieve the same effect as now. IIUC, there is no clear guidance when to put a check into the check command or the factory. 🤷 Anyway, I'd probably leave it as is.

@lucemia lucemia force-pushed the fix-8704-validate-source branch from 3b8c9ad to 03e11a1 Compare December 5, 2023 01:37
@lucemia
Copy link
Contributor Author

lucemia commented Dec 5, 2023

@radoering I have updated the implementation to support multiple-constraints dependency. Test case modified as well.

@lucemia lucemia requested a review from radoering December 5, 2023 01:50
@lucemia lucemia force-pushed the fix-8704-validate-source branch 2 times, most recently from 9596860 to 543268b Compare December 5, 2023 03:30
radoering
radoering reviewed Dec 9, 2023
View reviewed changes
@lucemia lucemia force-pushed the fix-8704-validate-source branch from 543268b to 14bc6de Compare December 9, 2023 13:47
@lucemia lucemia requested a review from radoering December 9, 2023 14:18
radoering
radoering reviewed Dec 10, 2023
View reviewed changes
@lucemia lucemia force-pushed the fix-8704-validate-source branch from 14bc6de to fd21a8c Compare December 11, 2023 01:55
@lucemia lucemia requested a review from radoering December 11, 2023 02:06
@radoering radoering force-pushed the fix-8704-validate-source branch from fd21a8c to 156267d Compare December 11, 2023 15:31
@radoering radoering changed the title Fix #8704 add source validation in poetry check add source validation in poetry check Dec 11, 2023
@radoering radoering merged commit f310a59 into python-poetry:master Dec 11, 2023
MrGreenTea pushed a commit to MrGreenTea/poetry that referenced this pull request Dec 18, 2023
@lucemia @MrGreenTea
add source validation in poetry check (python-poetry#8709)
f225594
@radoering radoering mentioned this pull request Feb 17, 2024
Merged
@github-actions
Copy link

github-actions bot commented Mar 3, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@radoering radoering radoering approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

poetry check does not consider the package source

2 participants

@lucemia @radoering