Update package to use Composer\Semver and more robust version sorting/comparing #34
Conversation
| $parser = new Parser; | ||
| return $parser->normalize(self::stripGitHash($version1)) | ||
| === $parser->normalize(self::stripGitHash($version2)); | ||
| } |
There was a problem hiding this comment.
Composer\Semver does not take kindly to versions containing git metadata tagged on to the end of it. This can be injected by Box, for example, when building PHARs. We use a small method to strip it out before passing to Semver.
|
|
||
| /** | ||
| * @param array $versions | ||
| */ | ||
| public function __construct(array $versions = array()) | ||
| { | ||
| $this->versions = $versions; | ||
| $this->parser = new Parser; |
There was a problem hiding this comment.
I recommend not omitting () during class instantiation without constructor parameters.
There was a problem hiding this comment.
It's not part of the PSR-2 standard which is enforced. We might adopt PSR-12 one day, if it ever leaves draft to get voted on, but I won't expect any contributor to follow a standard which we don't advertise/enforce upfront. That includes me and my own bias towards wondering why the seven hells I need to include parentheses when there are zero parameters ;).
There was a problem hiding this comment.
I remember the first time I saw this I was confused as I didn't know it was possible, and when you call a function foo which has no arguments you call foo(), not foo. So I did wonder why the seven hells people took the liberty to change it with new 
In any case, it's my own bias as well and I don't think anyone would feel strongly about it 😄
There was a problem hiding this comment.
That's why I don't like PSR-2 - too much room for improvement. Anyway please make it consistent across the code. If in all other places the () are not added, then don't added it here too.
| ) { | ||
| return true; | ||
| } | ||
| } catch (\UnexpectedValueException $e) { |
There was a problem hiding this comment.
UnexpectedValueException thrown by Composer\Semver. I should add a check that we do the following !== comparison only if the strategy in play is NOT github. Otherwise, we'll silently fail on invalid semver when there MUST be a valid semver version string.
In the event that a) we are using Github strategy, and b) Composer\Semver reports an invalid version string, throw an exception noting the problem which prevents comparing current version to new remote version.
Closes #31