Implemented lower and upper constraint bounds#75
Conversation
Codecov Report
@@ Coverage Diff @@
## master #75 +/- ##
============================================
- Coverage 96.89% 94.52% -2.37%
- Complexity 206 247 +41
============================================
Files 7 8 +1
Lines 386 475 +89
============================================
+ Hits 374 449 +75
- Misses 12 26 +14
Continue to review full report at Codecov.
|
|
Stopping the review because I notice from the tests it seems quite off from what we'd need to achieve. The operator of the constraint must be taken into account. |
|
So what's the actualy goal then here? I thought the goal was to get the normalized bounds from a contstraint so you can |
|
Yes that's the goal, but a constraint like While a constraint like |
|
I see. Closing then. |
65c3f71 to
2523df6
Compare
|
In case we agree that the current state is the correct way to go, I'd finish this by implementing it on the |
|
I think so :) |
|
I pushed a test in the branch (sorry broke 5.3 don't wanna push again if you're back working on it), and added another test locally but it fails so I thought I'll let you add it and try to make it pass: public function testMultipleMultiConstraintsMergingWithGaps()
{
$versionParser = new VersionParser();
$constraints = [
'^7.1.15 || ^7.2.3',
'^7.2.2',
];
foreach ($constraints as &$constraint) {
$constraint = $versionParser->parseConstraints($constraint);
}
$constraint = new MultiConstraint($constraints);
$this->assertSame(array('>=', '7.2.3.0.dev'), $constraint->getLowerBound(), 'Expected lower bound does not match');
$this->assertSame(array('<', '8.0.0.0.dev'), $constraint->getUpperBound(), 'Expected upper bound does not match');
}The lower bound returned is |
|
No prob, will add that test as well. |
|
@Seldaek your test is just wrong. The reference thing doesn't work because it would nest constraints and also the lower bound in that case is |
|
So I've removed the black magic, the additional interface is gone as well and we now have a proper |
|
I also got rid of the infinity thing and used |
|
You're right it should have been |
|
I fixed your test, yes :) |
e09b73c to
68340f8
Compare
|
After discussion with @Seldaek and @naderman, we agreed that the whole comparison logic is probably way too specific and out of scope for the semver component. |
| return $this->isInclusive; | ||
| } | ||
|
|
||
| public function isLowerMost() |
| return $this->getVersion() === '0' && $this->isInclusive(); | ||
| } | ||
|
|
||
| public function isUpperMost() |
There was a problem hiding this comment.
or if you go for isMin, isMax?
There was a problem hiding this comment.
rename to "isPositiveInfinity()"
| /** | ||
| * @return self | ||
| */ | ||
| public static function upperMost() |
There was a problem hiding this comment.
Should this be called positiveInfinity? or something? or max? "upperMost" is pretty weird.
There was a problem hiding this comment.
rename to "positiveInfinity()"
| /** | ||
| * @return self | ||
| */ | ||
| public static function lowerMost() |
| } | ||
|
|
||
| // Question we're answering here is "am I higher than $other?" | ||
| return '>' === $operator ? $other->isInclusive() : !$other->isInclusive(); |
There was a problem hiding this comment.
this looks weird to be that $this->isInclusive() is never taken into account. Is this correct ?
There was a problem hiding this comment.
tbh I don't know. I've been messing with this code for too long but there are lots of tests in the MultiConstraintTest which uses the compareTo() method internally. So maybe we were missing a constellation that requires a test? Would be great if you could provide that. More eyes is always better :)
There was a problem hiding this comment.
$this->isInclusive is implicitly checked when we do ($this == $other) above.
When we come to this part of the code, the version on both bounds is the same, so the following table is the result (x means, the bounds are identical, so the early return at the top of the function prevents this from mattering):
| expression | this.isInclusive | other.isInclusive | result |
|---|---|---|---|
| this.compareTo(other, >) | 0 | 0 | x |
| this.compareTo(other, >) | 0 | 1 | 1 |
| this.compareTo(other, >) | 1 | 0 | 0 |
| this.compareTo(other, >) | 1 | 1 | x |
| this.compareTo(other, <) | 0 | 0 | x |
| this.compareTo(other, <) | 0 | 1 | 0 |
| this.compareTo(other, <) | 1 | 0 | 1 |
| this.compareTo(other, <) | 1 | 1 | x |
So you can see that the expression is correct. But since we exclude a large part of the variations with early returns, an expression like '>' === $operator ? !$this->isInclusive() : $this->isInclusive() would be just as correct, since we already made sure that $this->isInclusive() != $other->isInclusive() once it gets to this section of the code.
Implementation for #72 :)