Bump nikolaposa/version version #61
Conversation
| @@ -30,7 +30,7 @@ public function fromRepository(CheckedOutRepository $checkedOutRepository) : Ver | |||
| ->mustRun() | |||
| ->getOutput(); | |||
|
|
|||
| return VersionsCollection::fromArray(array_filter( | |||
| return new VersionsCollection(...array_filter( | |||
There was a problem hiding this comment.
An array_values() is needed here. To test this, try an array of [1.0.0, 1.potato, 2.0.0] and you will get a TypeError if I remember correctly (test is to be added)
There was a problem hiding this comment.
Seems OK to me: https://3v4l.org/d5RvE
We test invalid at testFromRepositoryIgnoresInvalidVersions
There was a problem hiding this comment.
Hmm, interesting. I guess it crashes only when there are non-numeric keys then 👍
| @@ -19,8 +18,6 @@ | |||
| */ | |||
| public function forVersions(VersionsCollection $versions) : Version | |||
| { | |||
| Assert::that($versions->count())->greaterOrEqualThan(1); | |||
|
|
|||
| $versions->sort(VersionsCollection::SORT_DESC); | |||
There was a problem hiding this comment.
Relying on internal sorting is indeed risky, and I'm sure @nikolaposa will also make these collections immutable at some point, so it is probably a good idea to cast everything to an array via iterator_to_array() and operate on that afterwards.
There was a problem hiding this comment.
@Ocramius TBH I even planned to remove VersionsCollection class entirely in a newest 3.0.0 release, thinking it was unnecessary and nobody was using it. 😱 That's why I didn't give enough attention to its design. Feel free to open an issue describing fallacies of the class and I'll see what can be done. Of course, if you already have some idea for fixing it, open a PR along the way, I would be glad to accept it.
|
@ntzm I've implemented support for parsing As for the issue in question, feel free to open a issue (https://github.com/nikolaposa/version/issues), I would gladly consider fixing it. |
| @@ -280,7 +280,7 @@ public function testExecuteWithDefaultRevisionsNotProvided() : void | |||
| { | |||
| $fromSha = sha1('fromRevision', false); | |||
| $toSha = sha1('toRevision', false); | |||
| $versions = VersionsCollection::fromArray(['1.0.0', '1.0.1']); | |||
| $versions = new VersionsCollection(Version::fromString('1.0.0'), Version::fromString('1.0.1')); | |||
There was a problem hiding this comment.
@ntzm I see that this change caused some difficulties, because the ability to create the collection from version strings was very handy I would say now that I'm seeing use cases. Reasoning on my side was making things strict as possible, in order to avoid any ambiguity and simplify validation. So between string and Version inputs I decided to allow passing Version instances only.
I might consider introducing fromVersionStrings() and fromArray() named constructors for 3.1.0. Of course, feel free to suggest features you might find useful.
There was a problem hiding this comment.
I personally think it's a good change 👍
|
@nikolaposa I agree with you on the |
|
@nikolaposa I'd probably align to what composer can parse in tags |
|
About the |
|
This has been rebased and manually merged to |
Partially fixes #37, allows for
vprefix but notrelease-PHPStan is failing due to the return type for
VersionsCollection::getIteratorisTraversable, which doesn't have thecurrentmethod. Not sure what the best fix is here?Tests and code for checking if a
VersionsCollectionis empty have been removed, due toVersionsCollectionrequiring at least oneVersionin the constructor.