Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring #13

Merged
merged 17 commits into from Sep 2, 2015
Merged

Refactoring #13

merged 17 commits into from Sep 2, 2015

Conversation

alcohol
Copy link
Member

@alcohol alcohol commented Aug 3, 2015

Aims to:

  • expand code coverage
  • clean up version parser
  • clean up constraint classes
  • adds some methods for working with versions (satisifies, satisfiedBy, sort, rsort)

For the Semver::satisfies test I looked at npm/node-semver.

@alcohol alcohol changed the title Refactoring WIP - Refactoring Aug 3, 2015
### [Unreleased]

* Break: `VersionParser::parseNameVersionPairs` was removed.
* Changed: `VersionConstraint` now throws `InvalidArgumentException` instead of `Comparator::compare`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the wording looks weird here

@alcohol
Copy link
Member Author

alcohol commented Aug 7, 2015

Admittedly it got a bit larger than I intended, my apologies for that. However, over 60% of the changes are in the tests or new functionality. Because of the renaming and coupling between some of the changes I don't see an easy way to rebase this into several PR's.

@alcohol alcohol changed the title WIP - Refactoring Refactoring Aug 7, 2015
* allows (but ignores) build metadata now
* allows (but ignores) prefixing numeric versions with a v
* added some more descriptive inline comments
* enabled more test cases
@alcohol
Copy link
Member Author

alcohol commented Aug 19, 2015

/cc @naderman @stof @Seldaek @legoktm

When you have time, could you give some final feedback, please?

I'd like to see this wrapped up one way or another, so we can move towards an initial release (and start a PR for implementing it in composer/composer possibly).

@@ -9,45 +9,222 @@
* the LICENSE file that was distributed with this source code.
*/

namespace Composer\Test\Semver;
namespace Composer\Semver\Test;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👎

@alcohol
Copy link
Member Author

alcohol commented Aug 19, 2015

@GrahamCampbell if you are not going to explain your comments, please refrain from commenting at all.

'strips leading v' => array('v1.0.0', '1.0.0.0'),
'parses dates y-m as classical' => array('2010.01', '2010.01.0.0'),
'parses dates w/ . as classical' => array('2010.01.02', '2010.01.02.0'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really parsed as classical version if the leading zeroes remain in .01 and .02?

@Seldaek
Copy link
Member

Seldaek commented Sep 2, 2015

Merging, my only concern (beyond the comment above which is a nitpick but can be checked later..) is the removal of parseNameVersionPairs which should be reflected in composer, it needs to be moved somewhere else, I don't think there's a PR for that yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants