Uniform version style for root package #51
Conversation
src/PackageVersions/Versions.php
Outdated
| @@ -3,27 +3,80 @@ | |||
| namespace PackageVersions; | |||
|
|
|||
| /** | |||
| * This is a stub class: it is in place only for scenarios where PackageVersions | |||
There was a problem hiding this comment.
This file should not have been committed
There was a problem hiding this comment.
Damn, my bad! That's also probably the cause behind the current CI failure...
src/PackageVersions/Versions.php
Outdated
| 'symfony/process' => 'v3.3.13@a56a3989fb762d7b19a0cf8e7693ee99a6ffb78d', | ||
| 'theseer/tokenizer' => '1.1.0@cb2f008f3f05af2893a87208fe6a6c4985483f8b', | ||
| 'webmozart/assert' => '1.2.0@2db61e59ff05fe5126d152bd0655c9ea113e550f', | ||
| 'ocramius/package-versions' => '2.0.x-dev', |
There was a problem hiding this comment.
Uhm... This doesn't look right to me. The hash should be there anyway.
There was a problem hiding this comment.
The hash is there, that output was due to a previous iteration during my development where operator precedence tricked me. In fact I hadn't touched any other test apart from adding mine, so nothing has changed in expected behavior.
Jean85
force-pushed
the
uniform-package-version
branch
from
94305fd
to
8131db6
Compare
|
I've forced push the fix to avoid garbage in the Git history of the project. Now it should be good to go. PS: I was thinking about adding a |
src/PackageVersions/Installer.php
Outdated
| @@ -177,6 +177,6 @@ private static function getVersions(Locker $locker, RootPackageInterface $rootPa | |||
| ); | |||
| } | |||
|
|
|||
| yield $rootPackage->getName() => $rootPackage->getVersion() . '@' . $rootPackage->getSourceReference(); | |||
| yield $rootPackage->getName() => ($rootPackage->getPrettyVersion() ?? $rootPackage->getVersion()) . '@' . $rootPackage->getSourceReference(); | |||
There was a problem hiding this comment.
Which composer API introduced getPrettyVersion? Can you please make sure the constraints are correct for that to work?
There was a problem hiding this comment.
Sorry, I didn't check that because I thought that the --prefer-lowest of the CI covered that. I've tracked down the addition of that method to this commit from 2011: composer/composer@8e6f8ae#diff-d65a9c0319702b1659e4d5adce449e79
But I cannot find a source for the versioning of the API... where is it stored?
There was a problem hiding this comment.
Should be
Line 14 in 611925b
There was a problem hiding this comment.
Sorry, I didn't explain me well... I don't know where I can find some docs or reference for that API so I can track down the corresponding version to that Composer commit!
There was a problem hiding this comment.
The plugin API version seems to have been declared here in 2013:
composer/composer@92b1ee2#diff-9761e5e6cef580d24c5a05d870dcc479R30
It's two years after getPrettyVersion got added to the interface, so we should be ok! 👍
There was a problem hiding this comment.
@Jean85 what's the case when you expect $rootPackage->getPrettyVersion() to return NULL so that the code falls back to using $rootPackage->getVersion()? Given that the pretty version is the one obtained from the VCS, it should be always available. Otherwise, there's nothing to be further normalized.
If I'm wrong, and it's possible then this case probably should be covered by an additional test.
There was a problem hiding this comment.
I've added that because it's already covered by some test somewhere. If you try to remove that, you will see PHPUnit fail.
There was a problem hiding this comment.
Hmm... that test may as well be wrong then. Can you dig it up so we review it?
There was a problem hiding this comment.
I've commited the differences, see the latest commit of this PR: ca0fd61
I just had to update the mocks! I'll commit a small refactoring of the tests now...
src/PackageVersions/Installer.php
Outdated
| @@ -177,6 +177,6 @@ private static function getVersions(Locker $locker, RootPackageInterface $rootPa | |||
| ); | |||
| } | |||
|
|
|||
| yield $rootPackage->getName() => $rootPackage->getVersion() . '@' . $rootPackage->getSourceReference(); | |||
| yield $rootPackage->getName() => ($rootPackage->getPrettyVersion() ?? $rootPackage->getVersion()) . '@' . $rootPackage->getSourceReference(); | |||
|
@Jean85 seems like you'll need a rebase before I can merge this. |
|
Yep, the funny thing is that I de-synced it myself with #50 😄 |
|
@Jean85 I see you merged |
Jean85
force-pushed
the
uniform-package-version
branch
from
f964ad3
to
6db5757
Compare
|
@Ocramius sorry, I just used the web interface "Update branch" button, and I wasn't sure what it would do. Rebased 👍 |
|
Ok, I've fixed the mocks as per my comment and also pushed a small refactoring of the mock for the root package in the tests. |
In Jean85/pretty-package-versions#3 I was made aware of the fact that the root package version is dumped in a different, normalized style; this is due to the fact that the root package version is not retrieved from the lock file like the other deps, and in the lock file the versions are not normalized.
This PR uses the
PackageInterface::getPrettyVersion()from Composer to obtain the not-normalized version for the root package, obtaining a more uniform result.This is needed since my wrapper is used as a way to output the current version in PHARs (i.e. PHPStan), where the subject is the root package by design.