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

Improve caret translation logic #8

Merged
merged 7 commits into from
Dec 4, 2024
Merged

Conversation

jag250
Copy link
Contributor

@jag250 jag250 commented Dec 4, 2024

No description provided.

@jag250 jag250 requested a review from thatch December 4, 2024 19:01
@@ -213,7 +227,26 @@ def from_poetry_checkout(path: Path) -> bytes:
optional = False

if not version:
# e.g. git, path or url dependencies, skip for now
# e.g. git, path or url dependencies
if "path" in v:
Copy link
Member

Choose a reason for hiding this comment

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

Can you split this change out to a separate PR? This needs more thought and probably some sort of X- prefixed name, as I'm pretty sure they are not valid Requires-Dist.

else:
raise ValueError("All components were zero?")
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test with ^0 and make sure it does something predictable? I think it's invalid, and I don't know if it will raise (which is fine) or do something less useful like >=0,<1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That case would return >=0,<1. Similarly, ^0.0 returns >=0.0,<1.0 and ^0.0.0 returns >=0.0.0,<1.0.0. Is it better to raise?

Copy link
Member

Choose a reason for hiding this comment

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

There are two examples given at the bottom of https://python-poetry.org/docs/dependency-specification/#caret-requirements that are the best spec I can find. My personal read of

An update is allowed if the new version number does not modify the left-most non-zero digit in the major, minor, patch grouping.

is that this isn't a case they've really designed for. I'd think ^0.0.x is compatible with 0.0.xpost1 for example, but the prose implies that isn't the case.

I'm just saying, please add tests for the zero case if it returns now instead of raising.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check for this case that raises a ValueError!

@jag250 jag250 requested a review from thatch December 4, 2024 23:21
Copy link
Member

@thatch thatch left a comment

Choose a reason for hiding this comment

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

ok!

@thatch thatch changed the title Add git, path, url dependencies and fix caret translation logic Improve caret translation logic Dec 4, 2024
@jag250 jag250 merged commit 53f683f into python-packaging:main Dec 4, 2024
22 checks passed
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