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

Define parse_version for broad compatibility with packaging #32

Merged
merged 6 commits into from
Apr 19, 2023

Conversation

goerz
Copy link
Owner

@goerz goerz commented Apr 18, 2023

Closes #27

This will allow us to write a `parse_version` function that is
compatible with versions of `packaging` both before and after v22.0
@goerz goerz added the enhancement New feature or request label Apr 18, 2023
@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #32 (12d7c2a) into master (a3051b3) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
+ Coverage   96.46%   96.59%   +0.12%     
==========================================
  Files           6        7       +1     
  Lines         368      382      +14     
==========================================
+ Hits          355      369      +14     
  Misses         13       13              
Impacted Files Coverage Δ
src/docs_versions_menu/version_data.py 100.00% <ø> (ø)
src/docs_versions_menu/folder_spec.py 100.00% <100.00%> (ø)
src/docs_versions_menu/groups.py 100.00% <100.00%> (ø)
src/docs_versions_menu/parse_version.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

This restores the `LegacyVersion` behavior, which we depend on.
@goerz goerz marked this pull request as ready for review April 18, 2023 23:09
@goerz
Copy link
Owner Author

goerz commented Apr 18, 2023

Well, switching from packaging to packvers seems like it should do the trick.

@klauer, @awoimbee, @ZLLentz: Any comments? Should I merge and release this as v0.5.2?

@ZLLentz
Copy link

ZLLentz commented Apr 18, 2023

I'll drop these links for the context on packvers:
pypa/packaging#530 (comment)
pypa/packaging#530 (comment)

I think it's a good solution since it covers all the current use cases and doesn't require a big rewrite. It'll be nice to be able to install this alongside packages that require the most recent packaging versions 👍.

@klauer
Copy link
Contributor

klauer commented Apr 18, 2023

I admittedly don't fully understand the need to retain the legacy version parsing. However as it seems to be a crucial part of your workflow, @goerz, I think this is a mutually-agreeable solution and am on board 👍

By the way - we'll have to update the README indicating just how many repos use this library one day. In our organization alone, we have about 100 repositories using docs-versions-menu. Overall we're very happy with it. Thanks again for your great work here!

@goerz
Copy link
Owner Author

goerz commented Apr 19, 2023

I admittedly don't fully understand the need to retain the legacy version parsing. However as it seems to be a crucial part of your workflow, @goerz, I think this is a mutually-agreeable solution and am on board 👍

It's required because I'm putting all folder names in the gh-pages branch (or wherever the docs-versions-menucommand runs) through that parser. This is to sort and categorize the folder names. The folder names will usually be the names of tags (released versions like v0.1.0) or branch names. The LegacyVersion specifically handles all branch names (master/main, or whatever other branches might be set up to generate documentation). Thus, the name LegacyVersion is actually a bit confusing: this isn't about some "legacy" versioning convention, but about arbitrary branch names that aren't versions at all. It's probably a good idea to make the intent a bit more explicit, and hence I've added 34489f6.

By the way - we'll have to update the README indicating just how many repos use this library one day. In our organization alone, we have about 100 repositories using docs-versions-menu. Overall we're very happy with it. Thanks again for your great work here!

Wow! I'm glad it's getting some use!

Copy link

@awoimbee awoimbee left a comment

Choose a reason for hiding this comment

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

Thanks !

src/docs_versions_menu/parse_version.py Outdated Show resolved Hide resolved
goerz added 2 commits April 19, 2023 15:12
This allows us to go back to using `packaging` instead of `packvers` as
the dependency for parsing folder names/version numbers.

We'll have to depend on the internal `packaing.version._BaseVersion`,
but we can test the behavior that we rely on in case `packaging` makes a
breaking change to that behavior.
@goerz
Copy link
Owner Author

goerz commented Apr 19, 2023

Just for completeness, pypa/packaging#669 is quite interesting and points to packaging_legacy as yet another workaround for the removal of LegacyVersion. Alas, I'm perfectly happy to side-step the whole drama and implement the required functionality in docs_versions_menu directly. Hopefully, packaging will be more stable in the future, but if not, I'll just keep moving stuff into here.

Copy link
Contributor

@klauer klauer left a comment

Choose a reason for hiding this comment

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

I understand the issue better now and like the approach you chose here, sticking with packaging and extending it for the purposes of docs-versions-menu.

If/when the upstream library API changes, it can be revisited then.

@goerz goerz merged commit eaef171 into master Apr 19, 2023
@goerz goerz deleted the parse-version branch April 19, 2023 23:42
@ZLLentz
Copy link

ZLLentz commented Apr 19, 2023

The final approach here is clean and the code's intent is clear, I like it a lot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LegacyVersion has been removed from packaging.version
4 participants