Add Intervals class to allow checking for subsets and intersections between constraints #97
Conversation
…ing out all operator combinations
Seldaek
force-pushed
the
subsets
branch
2 times, most recently
from
b82b8e4
to
b894127
Compare
…s into Intervals class
|
Ok all tests passing except one now, which is technically a valid result but not really what I'd like it to return.. @Toflar if you'd like to try and use this latest version in your Composer PR feel free. |
… and avoid creating invalid intervals at all instead of filtering them afterwards
…>matches() on constraints
|
All tests now passing. 5aef5c2 adds a new isIntersectionOf method which could theoretically replace ConstraintInterface::matches() but I doubt it's as efficient so maybe not. We need to test and see. Anyway it was a great experiment as it allowed me to fix a few more edge cases in the intervals generation. @naderman would be happy to get your opinion on 67b8bb5 and 0b0edb3 especially |
Co-authored-by: Markus Staab <maggus.staab@googlemail.com>
Seldaek
changed the title
src/Intervals.php
Outdated
|
|
||
| // filter out invalid intervals like > x - <= x, or >= x - < x | ||
| if ( | ||
| version_compare($intervals[$index]['start']->getVersion(), $border['version'], '=') |
There was a problem hiding this comment.
Is it necessary to use version_compare even with normalized versions for equality?
There was a problem hiding this comment.
With normalized constraints no it is not needed I believe. We do have some tests using non-normalized constraints, and technically nothing preventing you from creating Constraint instances with non-normalized versions. I don't know if we want to just document this as "voiding warranty" and drop the version_compare for perf, or keep supporting these edge cases.
There was a problem hiding this comment.
BTW the failing tests that triggered me to add this were https://github.com/composer/semver/blob/master/tests/Constraint/ConstraintTest.php#L494 where b2 and beta2 do not compare equally without version_compare. Constraint::matches uses version_compare so technically it is supported to some extent, even though in Composer we never create non-normalized constraints AFAIK.
|
I still think it'd be better to have an actual Interval class rather than these arrays with start/end attributes which essentially act like an untyped object. |
| break 2; | ||
| } | ||
| // != dev-foo || == dev-master -> != dev-foo | ||
| if ($op === '==') { |
There was a problem hiding this comment.
this behaves incorrectly for anyDev, e.g. != dev-foo || == dev* -> * not != dev-foo
There was a problem hiding this comment.
Should probably also add a test that covers this situation.
|
Fixed all feedback. |
src/Intervals.php
Outdated
| */ | ||
| private static function generateIntervals(ConstraintInterface $constraint, $stopOnFirstValidInterval = false) | ||
| { | ||
| if ($constraint instanceof EmptyConstraint) { | ||
| return array('intervals' => array(array('start' => self::zero(), 'end' => self::positiveInfinity())), 'devConstraints' => array(self::anyDev())); | ||
| return array('intervals' => array(new Interval(Interval::zero(), Interval::positiveInfinity())), 'devConstraints' => array(Interval::anyDev())); |
There was a problem hiding this comment.
Imho we have the same optimization possibility here as we'd have with a ConstraintFactory: we don't need a new instance of Interval over and over again if the interval is exactly the same. We could reuse existing interval instances, right? :)
There was a problem hiding this comment.
Sure, but IMO that's still best done with the memoization on get(), because the expensive stuff here is computing intervals for a constraint, not storing them. Especially as they'll get used at poolbuilder time and then it can be cleared as it becomes useless. Feel free to PR if you have time though :P
No description provided.