search

IonBazan / composer-diff

Compares composer.lock changes and generates Markdown report so you can use it in PR description.
https://packagist.org/packages/ion-bazan/composer-diff
MIT License
143 stars 6 forks source link

Inconsistency when comparing dev- versions #5

Closed trakos closed 3 years ago

trakos commented 3 years ago

Hey, I've integrated your package in my gitlab's CI, and have dangerbot post it as a comment to MR. Thanks for putting it together, it's a great tool, and other similar packages don't support custom gitlab domain.

I've encountered one minor inconsistency when using it for unfinished MRs. The "Operation" column for packages with dev-* is hard to predict. To pinpoint it, I've added to test \IonBazan\ComposerDiff\Tests\Formatter\FormatterTest::testItRendersTheListOfOperations this cases:

new UpdateOperation($this->getPackage('a/package-6', '0.5.2'), $this->getPackage('a/package-6', 'dev-feature', 'dev-feature 1234567')),
new UpdateOperation($this->getPackage('a/package-7', '0.5.2'), $this->getPackage('a/package-7', 'dev-main', 'dev-main 1234567')),
new UpdateOperation($this->getPackage('a/package-8', '0.5.2'), $this->getPackage('a/package-8', 'dev-master', 'dev-master 1234567')),

and the relevant test results were:

Dev Packages Operation Base Target
a/package-6 Downgraded 0.5.2 dev-feature 1234567
a/package-7 Downgraded 0.5.2 dev-main 1234567
a/package-8 Upgraded 0.5.2 dev-master 1234567

I think it would make sense to consider operations with dev- either always show up as Upgraded, or show something neutral, like Changed?

The current behavior is caused by \Composer\Semver\VersionParser::normalizeDefaultBranch called in \Composer\Semver\Semver::sort - it does:

if ($name === 'dev-master' || $name === 'dev-default' || $name === 'dev-trunk') {
     return '9999999-dev';
}

Also, I think the composer implementation of version comparison always assumes that changes involving dev- are an upgrade, no matter the change. Maybe in \IonBazan\ComposerDiff\Formatter\AbstractFormatter::isUpgrade you could simply use composer's \Composer\Package\Version\VersionParser::isUpgrade?

IonBazan commented 3 years ago

Glad to hear you find this tool helpful! Thanks for the report, I was actually aware of this limitation because it's not possible to determine whether a change leading to or from specific commit hash is an upgrade or downgrade. Introducing another state Changed might be a nice way to cover that. I was thinking about refactoring and introducing a wrapper DTO on top of OperationInterface with some useful methods like isUpgrade, isChange, isStable, etc to avoid calling AbstractFormatter or PackageDiff class statically (breaking Demeter law).

The reason why we can't use \Composer\Package\Version\VersionParser::isUpgrade is that it was introduced in Composer v2 while this package tries its best to support both legacy and latest PHP and Composer versions. Hence some internal logic has been ported to support very outdated environments.

I will try to work on that later this week unless you are keen to provide a PR 😉