-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: handle || operator in version constraints for poetry add #10654
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?
Conversation
Reviewer's GuideAdjusts dependency requirement parsing to correctly treat spaced version constraints with operators like File-Level Changes
Assessment against linked issues
Possibly 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 there - I've reviewed your changes - here's some feedback:
- The extras regex character class
r"\[([\w\d,-_]+)\]$"is a bit confusing and potentially error-prone (e.g.\dis redundant and the-is in the middle of the class); consider simplifying to something liker"\[([-\w,]+)\]$"or a clearly ordered character class to avoid unintended ranges. - The logic for extracting extras from the name is now duplicated in both the initial constraint-regex branch and the space-based fallback; consider factoring this into a small helper to keep the parsing behavior consistent and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The extras regex character class `r"\[([\w\d,-_]+)\]$"` is a bit confusing and potentially error-prone (e.g. `\d` is redundant and the `-` is in the middle of the class); consider simplifying to something like `r"\[([-\w,]+)\]$"` or a clearly ordered character class to avoid unintended ranges.
- The logic for extracting extras from the name is now duplicated in both the initial constraint-regex branch and the space-based fallback; consider factoring this into a small helper to keep the parsing behavior consistent and easier to maintain.
## Individual Comments
### Comment 1
<location> `tests/utils/test_issue_10569.py:25` </location>
<code_context>
+ ("requirement", "expected"),
+ [
+ # Issue #10569: || operator with spaces
+ (
+ "cachy>=0.1.0 || <0.3.0",
+ {"name": "cachy", "version": ">=0.1.0 || <0.3.0"},
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `||` constraints without surrounding spaces and with irregular spacing
Since the regression is about handling `||` with varying spacing, the current parametrization is a bit narrow. Please extend the test matrix to include examples like:
- `"cachy>=0.1.0||<0.3.0"` (no spaces)
- `"cachy>=0.1.0 || <0.3.0"` (extra spaces)
- `"cachy >=0.1.0 ||<0.3.0"` (asymmetric spacing)
This will better protect the regex-based behavior and ensure the fallback space-splitting logic does not mis-handle these variants.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tests/utils/test_issue_10569.py
Outdated
| ("requirement", "expected"), | ||
| [ | ||
| # Issue #10569: || operator with spaces | ||
| ( |
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.
suggestion (testing): Add tests for || constraints without surrounding spaces and with irregular spacing
Since the regression is about handling || with varying spacing, the current parametrization is a bit narrow. Please extend the test matrix to include examples like:
"cachy>=0.1.0||<0.3.0"(no spaces)"cachy>=0.1.0 || <0.3.0"(extra spaces)"cachy >=0.1.0 ||<0.3.0"(asymmetric spacing)
This will better protect the regex-based behavior and ensure the fallback space-splitting logic does not mis-handle these variants.
|
I am afraid you may be on the wrong track. I have not checked in detail but if I grasp your fix correctly, you are trying to make reading constraints more tolerant. However, the issue in #10569 is that we write invalid constraints. Even if Poetry would be able to read them they are still invalid and other tools would fail ( |
…oetry#10569) Union constraints (e.g., `^4|^6`) cannot be represented in PEP 508 format because the `||` operator is not part of the standard. When writing such constraints to `project.dependencies` or `dependency-groups`, Poetry would produce invalid entries like `pytest (>=4,<5 || >=6,<7)` which would then fail to parse on subsequent commands. This fix: - Detects when a dependency constraint is a VersionUnion - Skips writing to project.dependencies/dependency-groups for such constraints - Shows a warning explaining the constraint uses union syntax - Writes only to tool.poetry.dependencies where the Poetry-specific syntax is valid 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
a4710ec to
cf6b376
Compare
PR Rewritten with Correct ApproachThanks @radoering for pointing out that the issue is about writing invalid constraints, not reading them. What ChangedThe previous approach tried to make Poetry more tolerant when reading
New ApproachThis PR now:
ExampleResult in [tool.poetry.dependencies]
pytest = "^4|^6"
# NOT added to project.dependencies (which would produce invalid PEP 508)TestsAdded two tests covering:
All 86 tests in |
bug_fix: - < code_change: - Hey there - I've reviewed your changes - here's some feedback: logic: - **suggestion (testing):** Add tests for `||` constraints without surrounding spaces and with irre... general: - I am afraid you may be on the wrong track style: - ## PR Rewritten with Correct Approach
Summary
Fixes #10569
RequirementsParser._parse_simpleto prioritize regex matching for standard constraints (e.g.,>=1.0) before checking for spaces to split the string||) are captured as a single version string associated with the package name, rather than incorrectly splitting the package nameTest Plan
tests/utils/test_issue_10569.pycovering:poetry add "cachy>=0.1.0 || <0.3.0"- the original issue case>=,<=,>,<,!=,~,^)tests/utils/test_dependency_specification.pypasstests/console/commands/test_add.pypass🤖 Generated with Claude Code
Summary by Sourcery
Handle version constraints with the || operator in requirement strings without breaking existing formats in the requirements parser.
Bug Fixes:
Tests: