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

PR: Sever the dependency on packaging.version (dependencies) #508

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

davvid
Copy link
Contributor

@davvid davvid commented Dec 14, 2024

Make qtpy more self-contained by implementing our own simple version parse() function instead of relying on packaging.version.

@davvid
Copy link
Contributor Author

davvid commented Dec 14, 2024

One (minor?) downside is that this add a new function to the public API. If y'all agree that this is a good idea in general but would prefer for it to be private (or named differently) I'll happily rename it to _parse_version(...) or something along those lines. I just used the original name to minimize the diffs.

Thanks again for qtpy, BTW. I really appreciate the effort that has gone into this project.

@coveralls
Copy link

coveralls commented Dec 14, 2024

Coverage Status

coverage: 90.017% (+0.3%) from 89.71%
when pulling 1fe2e9f on davvid:packaging-version
into 1d2a1ea on spyder-ide:master.

@ccordoba12
Copy link
Member

Hey @davvid, nice to see you contributing to Qtpy again!

Make qtpy more self-contained by implementing our own simple version parse() function instead of relying on packaging.version.

I don't see much of a problem with this but could you better motivate the rationale for it? Does packaging add too many transitive dependencies for Qtpy?

One (minor?) downside is that this add a new function to the public API. If y'all agree that this is a good idea in general but would prefer for it to be private (or named differently) I'll happily rename it to _parse_version(...)

I was about to say that: the function should be renamed _parse_version and perhaps moved to the _utils module (in that case it should be named just parse_version).

@dalthviz, what do think?

Thanks again for qtpy, BTW. I really appreciate the effort that has gone into this project.

Thanks for git-cola too! Big fan until this day.

@dalthviz
Copy link
Member

dalthviz commented Dec 16, 2024

I think its fine but I'm curious too about why is better to not have packaging as a dependency. The one suggestion that occurs to me is to allow to use the packaging functionality if available as well as adding it as an extra/optional dependency. So basically go with a fallback approach where an attempt to use packaging.version.parse is done and if that errors go with the logic proposed here

@dalthviz dalthviz added this to the v2.5.0 milestone Dec 16, 2024
@dalthviz dalthviz changed the title dependencies: sever the dependency on packaging.version PR: sever the dependency on packaging.version (dependencies) Dec 17, 2024
@dalthviz dalthviz changed the title PR: sever the dependency on packaging.version (dependencies) PR: Sever the dependency on packaging.version (dependencies) Dec 17, 2024
davvid added a commit to davvid/qtpy that referenced this pull request Dec 18, 2024
…arse() is not installed

Continue relying on packaging.version.parse() when it is installed.
Our internal parsing code is now used as a fallback instead.

Suggested-by: @dalthviz in spyder-ide#508
@davvid
Copy link
Contributor Author

davvid commented Dec 18, 2024

Thanks for reviewing. I've updated the commit message to provide a little more rational. The basic idea is that this makes qtpy easier to vendor and use in environments where users don't already have packaging.version installed. Subjectively, packaging does seem like a pretty large dependency to bring along (over 5.6k of python code) just for the single parse() function, so the main motivation is minimalism and only paying for what we use.

Not that it matters, but I've been patching this change into a vendored copy of qtpy for a while now and it seems better to improve qtpy itself so that everyone can benefit. Being able to have a runtime environment that does not depend on packaging-related utilities seems like a win to me.

I implemented @dalthviz's suggestion to have our simpler _parse_version() try to use packaging.version.parse() and only use its internal version as a fallback. We now catch an ImportError when packaging.version is unavailable. I did this as a separate commit in case y'all decide that the simpler approach of just owning the logic ourselves is overall better/simpler. I can trivially drop that commit in case y'all are swayed.

Unfortunately, I was not able to move _parse_version() into the _utils module. The reason is that it would introduce a circular import dependency. The top-level __init__.py needs to use this function, and the top-level package is imported by _utils.py itself, so moving it there would introduce a cycle.

Going through that exercise did make me notice that _utils.py was using absolute (instead of relative) imports, so I added another commit that swaps it over to use modern relative imports instead. I rebased that change so that it comes before any of the other changes since it's more-so a cleanup / prep commit. This seemed okay to keep in this PR but let me know if y'all would prefer to split that out to a separate PR instead.

Thanks for considering this change. Cheers.

davvid added a commit to davvid/qtpy that referenced this pull request Dec 18, 2024
…arse() is not installed

Continue relying on packaging.version.parse() when it is installed.
Our internal parsing code is now used as a fallback instead.

Suggested-by: @dalthviz in spyder-ide#508
@davvid davvid force-pushed the packaging-version branch 2 times, most recently from 80d5771 to 1de683e Compare December 18, 2024 20:50
davvid added a commit to davvid/qtpy that referenced this pull request Dec 18, 2024
…arse() is not installed

Continue relying on packaging.version.parse() when it is installed.
Our internal parsing code is now used as a fallback instead.

Suggested-by: @dalthviz in spyder-ide#508
Make qtpy more self-contained by implementing our own version `parse()`
function instead of relying on `packaging.version`.

This makes it `qtpy` completely independent so that it can be trivially
vendored and used in environments where `packaging` is not installed
on the user's machine.
…arse() is not installed

Continue relying on packaging.version.parse() when it is installed.
Our internal parsing code is now used as a fallback instead.

Suggested-by: @dalthviz in spyder-ide#508
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.

4 participants