-
Notifications
You must be signed in to change notification settings - Fork 259
fix creation of python_version markers from constraints with one digit (e.g. ">3,<4", "3.*")
#902
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
base: main
Are you sure you want to change the base?
fix creation of python_version markers from constraints with one digit (e.g. ">3,<4", "3.*")
#902
Conversation
Reviewer's GuideAdjusts normalization of python_version markers so single-digit constraints (e.g. '3', '3.*', '>3', '<=3') are expanded to correct version ranges, and extends tests to cover these semantics and marker intersections. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 found 2 issues, and left some high level feedback:
- The logic that appends '.0' for one-digit python_version constraints is duplicated for both '==' and '!=' branches; consider extracting this into a small helper (e.g.,
_normalize_major_only_version(version)) to make the intent clearer and avoid future divergence. - The transformation of
python_version > "3"into a constraint starting at>=3.1is subtle; adding an inline comment innormalize_python_version_markersexplaining why3.0is intentionally excluded would help future maintainers understand the reasoning behind this behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic that appends '.0' for one-digit python_version constraints is duplicated for both '==' and '!=' branches; consider extracting this into a small helper (e.g., `_normalize_major_only_version(version)`) to make the intent clearer and avoid future divergence.
- The transformation of `python_version > "3"` into a constraint starting at `>=3.1` is subtle; adding an inline comment in `normalize_python_version_markers` explaining why `3.0` is intentionally excluded would help future maintainers understand the reasoning behind this behavior.
## Individual Comments
### Comment 1
<location> `tests/packages/test_dependency.py:128-133` </location>
<code_context>
("<3.5.4", 'python_full_version < "3.5.4"'),
(">=3.5.4", 'python_full_version >= "3.5.4"'),
("== 3.5.4", 'python_full_version == "3.5.4"'),
+ ("==3.*", 'python_version >= "3" and python_version < "4"'),
+ (
+ "~2.7 || ^3.6",
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case for a bare major-range python constraint (e.g. ">=3,<4") turned into markers
The new parametrized cases for `python_versions` already cover `==3.*` and `~2.7 || ^3.6`. To better match the PR’s focus on single-digit constraints, please also add a case where `python_versions` is a bare major range (e.g. `">=3,<4"`) and assert that `to_pep_508()` produces the corresponding `python_version` marker. This will confirm that normalized ranges like `>=3,<4` are translated correctly, not just the `==3.*` shorthand.
```suggestion
("== 3.5.4", 'python_full_version == "3.5.4"'),
("==3.*", 'python_version >= "3" and python_version < "4"'),
(">=3,<4", 'python_version >= "3" and python_version < "4"'),
(
"~2.7 || ^3.6",
'python_version == "2.7" or python_version >= "3.6" and python_version < "4.0"',
),
```
</issue_to_address>
### Comment 2
<location> `tests/packages/utils/test_utils.py:180-189` </location>
<code_context>
('python_version == "3.6"', "~3.6"),
('python_version == "3.6.*"', "==3.6.*"),
('python_version == "3.6.* "', "==3.6.*"),
+ ('python_version == "3"', ">=3.0,<3.1"),
+ ('python_version == "3.*"', ">=3,<4"),
# !=
('python_version != "3.6"', "!=3.6.*"),
('python_version != "3.6.*"', "!=3.6.*"),
('python_version != "3.6.* "', "!=3.6.*"),
# <, <=, >, >= precision 1
('python_version < "3"', "<3"),
- ('python_version <= "3"', "<3"),
- ('python_version > "3"', ">=3"),
+ ('python_version <= "3"', "<3.1"),
+ ('python_version > "3"', ">=3.1"),
('python_version >= "3"', ">=3"),
# <, <=, >, >= precision 2
('python_version < "3.6"', "<3.6"),
</code_context>
<issue_to_address>
**suggestion (testing):** Extend coverage for major-only python_version comparisons, especially "!=" and mixed inequalities
To better exercise the new major-only handling, consider adding a few more parametrized cases:
* `python_version != "3"` → `"!=3.*"`, to mirror the equality case and verify `!=` with a major-only version.
* A range like `'>="3.0"'` with `'<="3"'` to confirm dotted vs single-digit upper bounds are treated consistently.
* (Optional) A whitespace variant for a major-only case, similar to the existing `"3.6.* "` entries, to confirm trimming still applies.
These would strengthen coverage of the normalization logic for single-digit versions across all operators.
Suggested implementation:
```python
('python_version == "3.6"', "~3.6"),
('python_version == "3.6.*"', "==3.6.*"),
('python_version == "3.6.* "', "==3.6.*"),
('python_version == "3"', ">=3.0,<3.1"),
('python_version == "3.*"', ">=3,<4"),
# !=
('python_version != "3.6"', "!=3.6.*"),
('python_version != "3.6.*"', "!=3.6.*"),
('python_version != "3.6.* "', "!=3.6.*"),
('python_version != "3"', "!=3.*"),
('python_version != "3 "', "!=3.*"),
# <, <=, >, >= precision 1
('python_version < "3"', "<3"),
('python_version <= "3"', "<3.1"),
('python_version > "3"', ">=3.1"),
('python_version >= "3"', ">=3"),
# mixed range with major-only upper bound
('python_version >= "3.0" and python_version <= "3"', ">=3.0,<3.1"),
# <, <=, >, >= precision 2
('python_version < "3.6"', "<3.6"),
```
If the test suite represents compound marker expressions differently (for example, using separate parameters or a different string format than `"python_version >= \"3.0\" and python_version <= \"3\""`), adjust that case to match the surrounding style. Also, if your normalization collapses the lower bound for `>= "3.0"` to `>=3`, you may need to update the expected string to `">=3,<3.1"` instead of `">=3.0,<3.1"`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…git (e.g. ">3,<4", "3.*")
baceedd to
ceee503
Compare
Resolves: python-poetry/poetry-plugin-export#354
Summary by Sourcery
Fix normalization of python_version markers for single-component and wildcard version constraints and update related marker intersection behavior.
Bug Fixes:
Tests: