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

Implement tools.Version (based on SemVer) #4963

Merged
merged 3 commits into from Apr 25, 2019

Conversation

Projects
None yet
3 participants
@jgsogo
Copy link
Member

commented Apr 12, 2019

Changelog: Feature: Create tools.Version with limited capabilities
Docs: conan-io/docs#1253

  • I'm using loose=True to allow incomplete versions like 1.2 (no patch component) or "2" (no minor nor patch)

closes #4875

Note.- Supersedes #4949

@ghost ghost assigned jgsogo Apr 12, 2019

@ghost ghost added the stage: review label Apr 12, 2019

@jgsogo jgsogo added this to the 1.15 milestone Apr 12, 2019

@jgsogo jgsogo requested a review from lasote Apr 12, 2019

@danimtb

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

I like the loose=True by default.

Regarding the prerelease, I 'd go for the implementation of the semver library. Maybe just adding another attribute so people can choose (same as version ranges)?

@lasote

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

Ok about the loose.
About the comparison, I would say Version("1.2.3-dev90") != "1.2.3". So Version("1.2.3-dev90") < Version("1.2.3") but Version("1.2.3-dev90") == Version("1.2.3")?? How could it be even possible?

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

Ok about the loose.
About the comparison, I would say Version("1.2.3-dev90") != "1.2.3". So Version("1.2.3-dev90") < Version("1.2.3") but Version("1.2.3-dev90") == Version("1.2.3")?? How could it be even possible?

I don't understand your comment, sorry. Just to make it clear: if we don't take into account things like prereleases/microversion/build/meta/... (like it is implemented right now), then:

        v1 = Version("1.2")
        v2 = Version("1.2-dev0")

        # Compare ALL the components
        self.assertTrue(v1.major == v2.major)
        self.assertTrue(v1.minor == v2.minor)
        self.assertTrue(v1.patch == v2.patch)
        # ... nothing more to compare

        # So...
        self.assertTrue(v1 == v2)
        self.assertFalse(v1 > v2)
        self.assertFalse(v1 < v2)

It makes sense because this Version object is "limited", it only exposes major, minor and patch components, the user cannot retrieve other components of the full string they used to create the object, those components are lost.

@lasote

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

It makes sense because this Version object is "limited", it only exposes major, minor and patch components,

For me, it doesn't make sense at all. One thing is exposing some getters and a different thing is comparing incorrectly on purpose. But let's make sense for everyone, it is a wrapper so it's very easy, implement the getters for the "prerelease" and the "build" and make the comparison correct for these fields too.

@lasote
Copy link
Contributor

left a comment

Submit the requested changes in the comment.

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

Ok! Included those fields in the wrapper, and now prerelease is part of the version precedence (according to the semver standard build doesn't affect to precedence)

@lasote
Copy link
Contributor

left a comment

I think the PR from emscripten has been merged here :S please fix

@jgsogo jgsogo force-pushed the jgsogo:tool/4875-semver-wrapper branch from 9c6dac0 to 0c2332a Apr 25, 2019

@jgsogo jgsogo referenced this pull request Apr 25, 2019

Merged

Docs for tools.Version() #1253

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

It should be ok now, also with docs

@lasote

lasote approved these changes Apr 25, 2019

@lasote lasote merged commit 2a039a9 into conan-io:develop Apr 25, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Apr 25, 2019

@jgsogo jgsogo deleted the jgsogo:tool/4875-semver-wrapper branch Apr 25, 2019

@jgsogo jgsogo referenced this pull request May 3, 2019

Closed

Implement tools.Version #4949

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.