Implemented lower and upper constraint bounds

[Toflar]

Implementation for https://github.com/composer/semver/issues/72 :)

[codecov-io]

Codecov Report

Merging #75 into master will decrease coverage by 2.36%. The diff coverage is 84.26%.

@@             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     

Impacted Files	Coverage Δ	Complexity Δ
src/Constraint/EmptyConstraint.php	71.42% <0.00%> (-28.58%)	7.00 <2.00> (+2.00)	:arrow_down:
src/Constraint/Bound.php	65.51% <65.51%> (ø)	17.00 <17.00> (?)
src/Constraint/Constraint.php	100.00% <100.00%> (ø)	38.00 <11.00> (+11.00)
src/Constraint/MultiConstraint.php	100.00% <100.00%> (ø)	27.00 <11.00> (+11.00)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 811c569...79ad211. Read the comment docs.

[Seldaek]

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.

[Toflar]

So what's the actualy goal then here? I thought the goal was to get the normalized bounds from a contstraint so you can version_compare() yourself, in which case the constraint is not needed.

[Seldaek]

Yes that's the goal, but a constraint like 1.0.0 has upper and lower bounds both 1.0.0.0, your patch doesn't do this.

While a constraint like ^2.3 has lower bound 2.3.0.0 and upper 2.9999999.9999999.9999999. Maybe I missed something here, but I don't really see how it allows us to do this?

[Toflar]

I see. Closing then.

[Toflar]

Okay, let's check again @Seldaek and @naderman :)

[Toflar]

In case we agree that the current state is the correct way to go, I'd finish this by implementing it on the MultiConstraint with some integration tests together with the VersionParser.

[Seldaek]

I think so :)

[Toflar]

Here we go, looks pretty good imho :) Ping @Seldaek @naderman

[Seldaek]

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 >=7.1.15.0.dev which is correct for the first constraint but not for the set of constraints.

[Toflar]

No prob, will add that test as well.

[Toflar]

@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 7.2.2.0.dev not 7.2.3.0.dev 😄 Adjusted in my latest try though.

[Toflar]

So I've removed the black magic, the additional interface is gone as well and we now have a proper Bound object that represents a bound and also does the comparison to another bound. Now I am happy 😄

[Toflar]

I also got rid of the infinity thing and used PHP_INT_MAX because otherwise everybody would need to handle the infinity thing and because there are no Composer libraries loaded in our autload.php case it would mean to compare to inf which is kind of nonsense.

[Seldaek]

You're right it should have been >=7.2.2.0.dev as lower bound, but I don't get what you mean by "The reference thing doesn't work because it would nest constraints". Anyway I see the new version returns 7.2.2 so I guess you fixed it?

[Toflar]

I fixed your test, yes :)

[Seldaek]

Well with the previous version I got this:

Failed asserting that Array &0 (
    0 => '>='
    1 => '7.1.15.0.dev'
) is identical to Array &0 (
    0 => '>='
    1 => '7.2.3.0.dev'
).

So clearly you fixed more than the test..

[Toflar]

No, that was because of your test 😄 Because you used references and the variables were named the same, the last constraint was a nested constraint again:

    public function testMultipleMultiConstraintsMergingWithGaps()
    {
        $versionParser = new VersionParser();
        $constraints = [
            '^7.1.15 || ^7.2.3',
            '^7.2.2',
        ];
        foreach ($constraints as &$constraint) {
            $constraint = $versionParser->parseConstraints($constraint);
        }

        // Here $constraint is still a reference to the last $constraint of the loop so it's nested again
        // I could have fixed it by doing this here:
        // $multiConstraint = new MultiConstraint($constraints);
        // but I went with the easier solution ;-)
        $constraint = new MultiConstraint($constraints);
    }

[Seldaek]

Ah that's what you were talking about with references, sorry totally forgot I put a & there so I didn't understand anything. Makes sense now!

Anyway.. you wanna work on #71 based on this? ;) I guess something dumping a set of version_compare statements (+ possibly some normalization code for the runtime version) for the bounds of a given constraint?

[Toflar]

Guess that's its own PR then.

[naderman]

@Seldaek #71 is meant as entirely separate thing that actually does precise matching, not meant to be used in combination with this PR here. @Toflar you could add a code generator for checking the bounds in this PR here I think.

[Toflar]

You mean something that outputs version_compare(...) && version_compare(...))?

[naderman]

@Toflar exactly :-)

[Seldaek]

@GrahamCampbell we have php-cs-fixer for this kind of stuff, so maintainers can fix things whenever. Please refrain from spamming CS comments.

[Toflar]

I was just gonna say, can we use the CS fixer on master and implement it in CI for that? I don't know the preferences of this project so...

[GrahamCampbell]

I don't see any code style checking on the CI, otherwise I'd have not commented.

[Seldaek]

I am against CI checks for code style, it's annoying. I run fixer every now and then to normalize things. No big deal.

[Toflar]

I would've let it run on every commit. I'm so used to it ;) Feel free to add your perferred config on master, I'll happily rebase then.

[Seldaek]

You can run php-cs-fixer fix if you want to fix things here.

[Toflar]

Ah, I didn't notice it was there all the time because the cs fixer is not a dev requirement ^^ Will do next time I push.

[Seldaek]

Yes I think if you know the version there is not much point in dumping the code you rather use the constraint class directly to check and move on.

[Toflar]

It was a long day...

[Seldaek]

I'm not saying you have to do this now :) a beer and sleep would be more appropriate at this point.

[Toflar]

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. I've updated the PR accordingly and rebased it onto master. It now contains only the bounds logic, so this should be ready for a final review. Failing tests seem unrelated (probably just needs a restart). Want me to squash or will you be squash merging anyway?