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

ENH: drop dependency to packaging, use tuples to parse and compare version numbers #3604

Closed

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Oct 18, 2021

PR Summary

replaces #3600. Addionally to reducing duplicated code:

  • code is less verbose
  • one less dependency
  • as a bonus, comparing tuples is 100x faster than parsing and comparing objects, though it should never really matter in practice

keeping this as a draft for now, conflicting PRs should be prioritised over this one:

@@ -357,20 +355,6 @@ class h5py_imports:
_name = "h5py"
_err = None

def __init__(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

dropping this completely because setup.cfg requires h5py >= 3.1 anyway

@neutrinoceros neutrinoceros added infrastructure Related to CI, versioning, websites, organizational issues, etc enhancement Making something better labels Oct 18, 2021
@neutrinoceros neutrinoceros force-pushed the drop_packaging_dep branch 3 times, most recently from f3b8841 to 7a67f0f Compare October 18, 2021 18:08
@neutrinoceros
Copy link
Member Author

@yt-fido test this please

@munkm
Copy link
Member

munkm commented Oct 18, 2021

Considering how little version comparisons happen in the codebase, is there really a major speedup here? It seems like the packaging import makes the code nicely readable, and I don't see the verbosity a downside in this case.

@neutrinoceros
Copy link
Member Author

neutrinoceros commented Oct 18, 2021

Considering how little version comparisons happen in the codebase, is there really a major speedup here?

definitely not, sorry if this wasn't clear.

It seems like the packaging import makes the code nicely readable

Matters of opinions on this point I guess. I find that tuple comparison is at least as readable, as well as more consistent with the idiomatic way to check Python's version at runtime using sys.version_info.

Historically, we used to rely on distutils.version.LooseVersion. That class was convenient because it was forgiving to version strings not following the semantic version pattern, and could basically compare any two strings we threw at it.
distutils is however deprecated from the standard library, which is why we migrated to packaging.version.parse, the recommended replacement, which was equally forgiving (see #3385).
Having packaging as an additional dependency however became less justified because they're planning to remove the flexibility of the packaging.version.parse function (by dropping packaging.version.LegacyVersion, equivalent to distutils.version.LooseVersion). Now we're using packaging.version.Version, which has none of the flexibility LooseVersion used to have (see #3494)

I figure, having one simple internal function to maintain looks more and more like an easier choice than aiming for the moving target that is version parsing in the ecosystem.

@matthewturk
Copy link
Member

I don't want this to be interpreted as suggesting we not do this PR, but I want to put out there that I am pretty ambivalent to changes like this. I'm not opposed, but like @munkm , I don't always understand the priority of it, and I think that maybe it's okay to let things like this slide.

@neutrinoceros
Copy link
Member Author

Well this isn't a hill worth dying on. If you guys don't see value in this I'll just close it.

@neutrinoceros neutrinoceros deleted the drop_packaging_dep branch October 18, 2021 20:37
@matthewturk
Copy link
Member

I mean, that really wasn't what I was getting at.

@neutrinoceros
Copy link
Member Author

It's fine. I have more open PRs than I can decently handle anyway, and this one wasn't going to be mergeable for at least a couple weeks. I'll consider reopening it if packaging bothers me again

@tony
Copy link

tony commented Jan 15, 2022

It's fine. I have more open PRs than I can decently handle anyway, and this one wasn't going to be mergeable for at least a couple weeks. I'll consider reopening it if packaging bothers me again

@neutrinoceros

Could I use your version_tuple (link) in libtmux? I am struggling with this in a PR of my own tmux-python/libtmux#348 prefer yours. Could you even PR the function to libtmux, or could I copy it in under MIT license and modify as needed?

P.S. I think you were on to something here, after pypa/packaging#465. Removing the reliance on packaging, seeing as it's flexibility isn't there anymore

@neutrinoceros
Copy link
Member Author

Hi @tony, thanks for asking. I'm happy to contribute this function to libtmux, I'll open a PR or join the discussion there if I need any guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Making something better infrastructure Related to CI, versioning, websites, organizational issues, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants