Pre-release numbers aren't ordered numerically. #24
Conversation
|
Also worth noting is that my build passed in Travis, so there were no tests for this at all. I haven't created new unit tests for this, but if I do, I'll add them to the pull request. |
|
Thanks for your pull request @BrunoDEBARROS. I did never realize this problem. However, there are a few small problems with your fix. I'll put those in the diff |
| if ($v1->getTag() < $v2->getTag()) | ||
| return false; | ||
|
|
||
| if (!isset($t[$v1->getTag()]) && !isset($t[$v2->getTag()])) { |
There was a problem hiding this comment.
- Variable
$tis never defined. It was in an older version.
Because I improved tag handling, it is no longer necessary to check for the cases '' or '-' or '--', because they are simply never created on tags. - Is this condition necessary? Isn't it easier to first check for no tags and then return false (as they are equal and not greater)
|
I think it looks pretty good. It would be nice if you could add some unit tests, but if you don't have time, i'll have a look at it next week. |
|
Added a test to your regressionTest. I noticed that for issue #23 you changed quite a few of the tests, but as far as I know, it was just updating tests that rely on old behavior, so I'm not sure if you want tests to be added elsewhere, or if you want more test cases for this issue. Let me know. |
|
I think one test is enough to cover this case. |
Pre-release numbers aren't ordered numerically.
The following returns true:
When it really shouldn't. I used PHP's natsort() as a quick fix for this (it sorts the two tags naturally). I'm not sure if this fits in well with how you do things, so feel free to change it in whatever way you see fit, but it has solved my problem.