-
Notifications
You must be signed in to change notification settings - Fork 260
fix parsing Windows server platform_release
#911
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
fix parsing Windows server platform_release
#911
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts version parsing patterns and tests so that Windows Server-like platform_release strings (e.g. '2022Server') are rejected by default as invalid marker constraints but can be parsed when non‑PEP 440 parsing is explicitly requested, aligning behavior with existing non-standard versions like '4.9.253-tegra'. Flow diagram for version parsing with updated release patternflowchart LR
VersionString["Input version string"] --> ModeCheck{PEP440 parsing?}
ModeCheck -- Yes --> Pep440Parser["PEP440Parser"]
Pep440Parser --> Pep440Match{Matches PEP440?}
Pep440Match -- Yes --> Pep440Valid["Accept version as valid PEP440"]
Pep440Match -- No --> Pep440Invalid["Reject as invalid marker constraint (e.g. 2022Server)"]
ModeCheck -- No (non PEP440) --> NonStandardParser["NonStandardVersionParser"]
NonStandardParser --> ApplyPattern["Apply RELEASE_PATTERN with optional sign before build"]
ApplyPattern --> ReleaseGroup["Match release group: [0-9]+(.[0-9]+)*"]
ReleaseGroup --> OptionalBuild{Build part present?}
OptionalBuild -- No --> NonPep440ValidRelease["Accept plain numeric releases"]
OptionalBuild -- Yes --> BuildGroup["Match build group: [0-9a-zA-Z-]+(.[0-9a-zA-Z-]+)*"]
BuildGroup --> SignOptional["Optional + or - before build (e.g. 4.9.253-tegra)"]
SignOptional --> NonPep440Valid["Accept as valid non PEP440 version"]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
platform_releasesplatform_release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- Making the '+'/'-' separator optional in
RELEASE_PATTERNbroadens what will be parsed as a build segment (e.g.1.0abc), so it would be good to double‑check that this still matches the intended grammar and doesn’t unexpectedly change behavior for other version shapes. - In
test_parse_marker_constraint_does_allow_invalid_version_if_requested, theexpectedtuple plus*expected[:-1]andtype: ignore[misc]is a bit opaque; consider constructing theVersionexplicitly per case or using a small helper to avoid the ignore and make the mapping from input to expectedVersionclearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Making the '+'/'-' separator optional in `RELEASE_PATTERN` broadens what will be parsed as a build segment (e.g. `1.0abc`), so it would be good to double‑check that this still matches the intended grammar and doesn’t unexpectedly change behavior for other version shapes.
- In `test_parse_marker_constraint_does_allow_invalid_version_if_requested`, the `expected` tuple plus `*expected[:-1]` and `type: ignore[misc]` is a bit opaque; consider constructing the `Version` explicitly per case or using a small helper to avoid the ignore and make the mapping from input to expected `Version` clearer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
9632fab to
0f8dcb3
Compare
0f8dcb3 to
5417878
Compare
Resolves: python-poetry/poetry#10710
Summary by Sourcery
Relax version parsing to support additional non-PEP 440 platform release formats while preserving strict validation by default.
Bug Fixes:
Tests: