-
Notifications
You must be signed in to change notification settings - Fork 278
Return False instead of raising for .contains with invalid version
#932
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
Conversation
|
I'm supportive of this PR, I believe it makes packaging closer to PEP 440 compliant, as invalid versions are expressible via arbitrary equality. You've added tests for |
|
Any movement on this and/or #931? I think these are relatively straightforward changes; and if you'd like to see a different approach instead then I'm happy to do so. I'd love to contribute more to |
|
It occurs to me that his will likely need a follow up PR where these method handle arbitrary equality, I've wrote up an issue #943 |
|
Hi @Liam-DeVoe I just recently became a packaging maintainer and I am willing to review and accept this. Unfortunately though I just merged another PR that has caused this PR to have conflicts, if you are okay fixing those conflicts I will review afterward. |
|
Congrats! Yes of course, happy to! |
|
(that CI error is a real mypy warning that should have been an error if I had written better tests - I'll investigate and add a covering test) |
|
I'm actually confused as to why #767 says that
I think this is what you were pointing out as well in #943 (comment). I can make this PR return |
|
@Liam-DeVoe I agree, it's up to you, you can either fix it or I will make a follow up PR, once I am done with the pip 25.3 release, that fixes it. |
|
I don't think I'll have immediate time to dig into this in the next few days, so I would say you should go for it. |
|
Thanks @Liam-DeVoe for your contribution to packaging. |
|
Thank you Damian! I'm hopeful I can continue to contribute where it makes sense. |
|
@Liam-DeVoe I've found an interesting edge case adding support for arbitrary equality of arbitrary strings: SpecifierSet("===foobar,!=1.0").contains("foobar")Should that be Specifier("!=1.0").contains("foobar")I'm tempted to say yes for both and special case |
|
I've made #954 that change the behavior you implemented here, just for empty specifiers and not equal to specifiers. |
|
I'm almost tempted to propose that arbitrary equality clauses should not be allowed to appear in the same specifier set as any other clause (ie it must appear alone). But if ruling out the combination of arbitrary equality clauses with other clauses is not an option, then: I don't think it's sane to have I know this breaks this very nice invariant:
But I don't think it's too bad to repair. I would state the following two invariants:
Though this second invariant needs some work, e.g. what happens when a specifier set includes two different arbitrary equality clauses. I think disallowing multiple arbitrary equality clauses in the same specifier set would be a fine solution. |
PEP 440 says nothing about specifier sets, so the semantics of sets is the prerogative of >>> SpecifierSet('===1.0,>0.5').contains('1.0')
True
I am okay with the choice that the specifier But I am not okay to say that If the goal is to not allow users to include arbitrary strings I think we should go back to them throwing an exception and then users can prevalidate their versions with
I don't think this second invariant holds, but even if it did it still wouldn't achieve my other main goal. I want to be able to consider specifier sets without considering what versions they act on, because I want to be able to implement resolution algorithms that can do some static analysis on the specifiers, and looking up what versions available is potentially an IO call. I've already spent a lot of time reading the spec and conforming |
Closes #767.