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

Update per comment on PR #629 . Sort in a version-aware manner #829

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@kata198
Contributor

kata198 commented Oct 25, 2016

Update per comment on PR #629 . Sort in a version-aware manner such that 2.7.10 comes AFTER 2.7.2, not before.

This is in response to comment on PR #629 #629 (comment)

This changes the sort to a version-aware method, copied from my project "cmp_version" (https://github.com/kata198/cmp_version ). That project is Public Domain, so there are no licensing concerns.

Enjoy!

  • Tim
Update per comment on PR #629 . Sort in a version-aware manner, such …
…that 2.7.10 comes AFTER 2.7.2, not before.
@kata198

This comment has been minimized.

Contributor

kata198 commented Oct 25, 2016

Note this copies-and-pastes code from cmp_version instead of adding it as a dependency, I don't think we should add a dependency to setuptools. If you feel it belongs in another file ( I do, but I'm not sure where...), feel free ot modify accordingly.

@kata198

This comment has been minimized.

Contributor

kata198 commented Oct 25, 2016

I went ahead and moved cmp_version to the _vendor directory/module . Seems an apt location.

kata198 added some commits Oct 25, 2016

@jaraco

This comment has been minimized.

Member

jaraco commented Oct 28, 2016

I like the idea that you're keeping the _vendor code separate - that's a good plan.

Since cmp_version is a published package, it's probably best if we declare it as a vendored dependency - that is, in pkg_resources/_vendor/vendored.txt, declare the dependency for cmp_version, update pavement.py, and then run paver update_vendored (which is how all of the vendored dependencies are incorporated). Finally, instead of importing from pkg_resources._vendor, import from pkg_resources.extern. Doing this enables other package managers to treat cmp_version as a proper dependency and omit it from the vendored installation.

But more importantly - pkg_resources already relies on packaging, and I'd be really surprised if packaging doesn't already provide a suitable sort-order for these package version numbers.

>>> from pkg_resources.extern import packaging
>>> packaging.version
<module 'pkg_resources.extern.packaging.version' from '/Users/jaraco/Dropbox/code/main/setuptools/pkg_resources/_vendor/packaging/version.py'>
>>> low_ver = packaging.version.parse('2.7.2')
>>> high_ver = packaging.version.parse('2.7.10')
>>> low_ver < high_ver
True

Why not simply use that instead?

@kata198

This comment has been minimized.

Contributor

kata198 commented Oct 28, 2016

If you want, that's fine. I simplify included my cmp_version implementation
because I've been using that for a long time with python packages and I
know it works. The original implementation (without version-aware sorting)
adequately resolves the issue for me.

On Fri, Oct 28, 2016 at 1:41 PM, Jason R. Coombs notifications@github.com
wrote:

I like the idea that you're keeping the _vendor code separate - that's a
good plan.

Since cmp_version is a published package, it's probably best if we declare
it as a vendored dependency - that is, in pkg_resources/_vendor/
vendored.txt, declare the dependency for cmp_version, update pavement.py,
and then run paver update_vendored (which is how all of the vendored
dependencies are incorporated). Finally, instead of importing from
pkg_resources._vendor, import from pkg_resources.extern. Doing this
enables other package managers to treat cmp_version as a proper dependency
and omit it from the vendored installation.

But more importantly - pkg_resources already relies on packaging, and I'd
be really surprised if packaging doesn't already provide a suitable
sort-order for these package version numbers.

from pkg_resources.extern import packaging
packaging.version
<module 'pkg_resources.extern.packaging.version' from '/Users/jaraco/Dropbox/code/main/setuptools/pkg_resources/_vendor/packaging/version.py'>
low_ver = packaging.version.parse('2.7.2')
high_ver = packaging.version.parse('2.7.10')
low_ver < high_ver
True

Why not simply use that instead?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#829 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIEbO7JQrf85Q2rQCkulMWW4-iZGWQbXks5q4jOtgaJpZM4KfnO8
.

@jaraco

This comment has been minimized.

Member

jaraco commented Nov 4, 2016

Okay. Thanks for this, but I think we're better off seeking a solution that doesn't involve bringing in more implementations of version parsing. Setuptools only recently deprecated its own version parsing logic for the packaging module. Appreciate the effort anyhow. I should be able to use your suggestion to devise another solution.

@jaraco jaraco closed this Nov 4, 2016

@jaraco

This comment has been minimized.

Member

jaraco commented Nov 4, 2016

Hmm. As I'm looking at it, I see that you're passing full filenames to the VersionString constructor, but VersionString and cmp_version only talk about taking version strings (i.e. "1.0.0b1" and not filenames like "mypkg-1.0.0b1.egg"). Perhaps it would have worked anyway because it was treating 'mypkg-1' as one part of the version. That makes me think it still might not handle the sorting of 'mypkg-10.0.0' and 'mypkg-2.0.0' properly.

jaraco added a commit that referenced this pull request Nov 4, 2016

Use packaging.version.Version to sort filenames by the version of the…
… package they represent. Alternate implementation of that proposed in #829. Also ref #629.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment