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

Introduce major, minor and micro aliases to Version #225

Merged
merged 1 commit into from
Nov 4, 2019

Conversation

fridex
Copy link
Contributor

@fridex fridex commented Oct 23, 2019

No description provided.

@di
Copy link
Sponsor Member

di commented Oct 24, 2019

I'm not sure I agree with the the behavior here. A version like 2 and 2.0.0 are equivalent:

>>> from packaging.version import Version
>>> Version('2') == Version('2.0.0')
True

I think I would expect Version('2').minor to return 0, not None.

@fridex fridex force-pushed the introduce-major-minor-micro-versions branch from d6e7963 to 696fc7a Compare October 24, 2019 16:05
@fridex
Copy link
Contributor Author

fridex commented Oct 24, 2019

I'm not sure I agree with the the behavior here. A version like 2 and 2.0.0 are equivalent:

>>> from packaging.version import Version
>>> Version('2') == Version('2.0.0')
True

I think I would expect Version('2').minor to return 0, not None.

Thanks for the review. Adjusted accordingly.

@brettcannon
Copy link
Member

@fridex do note that CI failed.

@pradyunsg
Copy link
Member

pradyunsg commented Oct 25, 2019

Since these names are not the same as SemVer, I'm pretty sure that wouldn't be a source of confusion when using this API.

Also, I don't think the PEP endorses using these names -- that's not a reason to avoid adding these, but I don't think we should view the use for explanation as "this should definitely exist as functionality in packaging".


All that said, I'm on board for adding these aliases since they follow a common/clear convention, won't be a big maintenance burden, are unambiguous and useful for end users.

Thanks for suggesting this feature and filing this PR @fridex! ^>^

@pradyunsg pradyunsg changed the title Introduce minor, major and micro aliases as stated in PEP-440 Introduce minor, major and micro aliases to Version Oct 25, 2019
@pradyunsg pradyunsg changed the title Introduce minor, major and micro aliases to Version Introduce major, minor and micro aliases to Version Oct 25, 2019
@fridex fridex force-pushed the introduce-major-minor-micro-versions branch from 696fc7a to f1b2b22 Compare October 25, 2019 08:56
@fridex
Copy link
Contributor Author

fridex commented Oct 25, 2019

@fridex do note that CI failed.

Thanks for pointing it out, I missed it. Tests are green now.

Since these names are not the same as SemVer, I'm pretty sure that wouldn't be a source of confusion when using this API.

Also, I don't think the PEP endorses using these names -- that's not a reason to avoid adding these, but I don't think we should view the use for explanation as "this should definitely exist as functionality in packaging".

All that said, I'm on board for adding these aliases since they follow a common/clear convention, won't be a big maintenance burden, are unambiguous and useful for end users.

Thanks for suggesting this feature and filing this PR @fridex! ^>^

Thanks for the review. I adjusted the commit message not to mention PEP explicitly.

@di
Copy link
Sponsor Member

di commented Oct 25, 2019

Since these names are not the same as SemVer

Well, any reason not to add patch as an equivalent of micro and make them support SemVer?

@fridex
Copy link
Contributor Author

fridex commented Oct 25, 2019

Since these names are not the same as SemVer

Well, any reason not to add patch as an equivalent of micro and make them support SemVer?

I've added also patch property as requested. Tests are greet, PTAL.

@pradyunsg
Copy link
Member

@fridex Could you please revert 1988d11? I have some reservations toward that and I don't want that to block the rest of the changes here.

Let's do a separate follow up PR for adding the patch property, where we can have a dedicated discussion on that. :)

@fridex
Copy link
Contributor Author

fridex commented Nov 4, 2019

@fridex Could you please revert 1988d11? I have some reservations toward that and I don't want that to block the rest of the changes here.

Let's do a separate follow up PR for adding the patch property, where we can have a dedicated discussion on that. :)

Sure, I've just opened a separate PR which depends on this one in #229. PTAL

@pradyunsg pradyunsg merged commit bb80ca7 into pypa:master Nov 4, 2019
@pradyunsg
Copy link
Member

(@brettcannon I have assumed that you're ambivalent to this change, lemme know if you're opposed to this)

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

Successfully merging this pull request may close these issues.

None yet

4 participants