-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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: check for mutually exclusive markers #7970
fix: check for mutually exclusive markers #7970
Conversation
7a57db5
to
27fee87
Compare
this is obviously pulling in the exact opposite direction than #7257 poetry itself used overlapping markers until very recently - Lines 80 to 84 in c4c857e
presumably forbidding something that is currently allowed will be a breaking change for anyone who is doing that thing |
That goes too far. There are such cases. The most prominent one is something like: Lines 80 to 84 in c4c857e
Further, see the opencv example in #7257 |
ah yes, that's the other thing. poetry could forbid overlapping markers at the top level, but it's allowed for dependencies to declare sub-dependencies with overlapping markers anyway. So forbidding top-level overlaps doesn't achieve very much, right? |
While agree that a solution to better handle overlapping markers is to be implemented since that's commonly found in community packages, I would still find beneficial not to encourage writing others. Overlapping markers sound just a bad practice to me. That said, it's 2 core contributors vs me 😄 so I won't fight to hard if you are opinionated on this, and close the PR. |
I agree that it's often bad practice but I don't think it's always bad practice, e.g. in the example with a general constraint and an additional restriction depending on markers it avoids repetition (and people failing in their attempt to build the inverse marker). A check that should not be controversial is: If intersection of markers is not empty, intersection of version must not be empty. Afaik, we are already doing this during dependency resolution. Not sure if there is any value to do it earlier. |
Sounds like it's best to just close this then ;) |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Pull Request Check List
Resolves: #5066
I might be mistaken, but I don't see cases where one wants to specify multiple constraints for the same dependency where markers have an intersection.