-
Notifications
You must be signed in to change notification settings - Fork 278
Support arbitrary equality on arbitrary strings for Specifier and SpecifierSet's filter and contains method.
#954
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
|
Okay I've tweaked the semantics a little bit, and updated the PR description, based on the discussion with @Liam-DeVoe in #932 (comment). Any standard operator, i.e. not >>> SpecifierSet('===foobar,!=1.0').contains('foobar')
FalseThis is because The empty specifier set still selects arbitrary string versions for the reasons I outline in the description. I also realized there was an edge case around the empty specifier when passed arbitrary string versions and pre-release versions, I have updated the semantics so that non-PEP 440 arbitrary strings do not exclude pre-releases from being selected by the empty specifier e.g.: >>> list(SpecifierSet('').filter(['foobar', '1.0.a1']))
['foobar', '1.0.a1']Overall I think this is correct, and it reduces the semantic changes between this PR and what #932 implemented. |
|
Pinging @uranusjr @di and @pradyunsg based on #336 being the only significant discussion of arbitrary equality I could find, to check if anyone has any objections to this PR. |
|
I am going to wait one more week for objection, or reviews, and then merge this on the basis it is further completing PEP 440 implementation of Specifiers. My only concern of semantics is case sensitivity, and I don't think that choice should be made in this PR, so I've broken out a separate issue to address that: #974 |
|
I'd be happy to review the code, but I don't want to make a decision on changes, at least one like case insensitivity on |
a7adc9a to
a09de11
Compare
|
I've updated the PR and PR description to remove the concern on case insensitivity now that is decided: |
Specifier and SpecifierSetSpecifier and SpecifierSet's filter and contains method.
0fbafa1 to
5942e76
Compare
|
I’ll try to review this by tomorrow. Ping me if I forget. |
5942e76 to
96fc297
Compare
96fc297 to
b1fbe73
Compare
henryiii
left a comment
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.
Code looks good; I don't write resolvers, so don't weight my review of the idea too highly. On your choices:
- The invariant seems quite useful, and logical inference on specifiers is quite important, sounds reasonable.
- Reasonable.
- Even without the base class, adding None, as long as it behaves close enough to False, is pretty safe. Very few uses compare with
is False. Including the fact the base class allows None, I think it's quite safe. - I agree that option 1 sounds fine, simple is good, no strong opinion though.
Towards #943
According to the spec:
This enables that main purpose of arbitrary equality for
Specifier.contains,Specifier.filter,SpecifierSet.contains, andSpecifierSet.filter.There are four design choices I have made that are either ambiguous in the spec or purely a question for the
packagingAPI:1. Empty
SpecifierSetis satisfied by non-PEP 440 versionsThis is a natural consequence of wanting the invariant:
Given this invariant the empty specifier MUST allow non-PEP 440 versions, or you end up with
SpecifierSet("").contains("foobar")beingFalseand then adding the specifierSpecifierSet("===foobar").contains("foobar")beingTruebreaking the invariant.While this invariant isn't part of the specification, it is very hard to do any kind of logical inference if it is not true, and it does not violate the specification.
2. Any operator other than
===exclude non-PEP 440 versionsAs the operators
~=,==,!=,<=,>=,<, and>are only defined to work on PEP 440 style versions they exclude a non-PEP 440 version.3. For
Specifier.prereleaseswhen a version is not PEP 440 complaint returnNoneThis is an arbitrary choice, and technically changes the
Specifier.prereleasesAPI which previously guaranteed aboolreturn (though it's parent classBaseSpecifieralready allows for aNonereturn on this method), but I don't think eitherTrueorFalsemake sense.4. Explicitly setting
prereleasesdoes not affect the satisfiability of non-PEP 440 versionsThis is an arbitrarily choice, and not determined by the spec but by these options being available in the
packagingAPI.Being that arbitrary strings have no concept of pre-releases to be consistent either:
prereleasesdoes not affect the satisfiability of non-PEP 440 versionsprereleasesdoes not allow any non-PEP 440 versionsMy choice here was largely driven by the simplicity of the implementation, "1." is simple to implement, "2." is very complex and would likely require a major refactoring.