Skip to content

Improve caret translation logic - #8

Merged
jag250 merged 7 commits into
python-packaging:mainfrom
jag250:main
Dec 4, 2024
Merged

jag250 merged 7 commits into
python-packaging:mainfrom
jag250:main

Conversation

@jag250

@jag250 jag250 commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@jag250
jag250 requested a review from thatch December 4, 2024 19:01
Comment thread metadata_please/source_checkout.py
Comment thread metadata_please/source_checkout.py Outdated
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
Copy Markdown
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.

version_parts[-1] += 1

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@thatch thatch left a comment

Copy link
Copy Markdown
Member

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
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