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

Add package resources version fallback #101

Merged
merged 4 commits into from
Jan 25, 2023
Merged

Conversation

banesullivan
Copy link
Owner

Fixes an issue where our typical methods of uncovering a version fail for some packages by falling back to package-resoources if available to get the distribution version.

There may be justification for making this the default, but for now, I have tagged it on as a final fallback.

One particular package that needs this is pythreejs.

Main:

$ scooby pythreejs

--------------------------------------------------------------------------------

...

  Python 3.9.13 (main, Aug 25 2022, 18:24:45)  [Clang 12.0.0 ]

         pythreejs : Version unknown

This branch

$ scooby pythreejs

--------------------------------------------------------------------------------

...

  Python 3.9.13 (main, Aug 25 2022, 18:24:45)  [Clang 12.0.0 ]

         pythreejs : 2.4.1

assert version == scooby.report.VERSION_NOT_FOUND
assert version == "0.1.0" # Assuming this package never updates
Copy link
Owner Author

Choose a reason for hiding this comment

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

@prisae, I assume we won't ever update this package?

The changes in this PR make it tough to not get a version for a package pkg_resources will get the distribution version which has to be set afaik

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am absolutely not sure I like this. So that package has no version. That is the whole point of that package. So scooby should not print "0.1.0", because it is simply wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, pip does assign it by default 0.1.0. That is nothing I did, so I would not "update" the version number, it does not has one. Pip gives it.

If the package would not be installed, but in the python path, then scooby would still return Version unknown (hence scooby.report.VERSION_NOT_FOUND), so I think we should test that.

@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2023

Codecov Report

Merging #101 (bfe4560) into main (5150815) will decrease coverage by 0.03%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##             main     #101      +/-   ##
==========================================
- Coverage   85.61%   85.58%   -0.03%     
==========================================
  Files           5        5              
  Lines         424      437      +13     
==========================================
+ Hits          363      374      +11     
- Misses         61       63       +2     

@banesullivan
Copy link
Owner Author

Pre-commit runs fine for me locally. I really need (want) this change, so I'm merging and releasing.

These changes render much of the knowledge.py module useless as we don't need version attributes or version setting methods and can just rely on pkg_resources. I'm going to PATCH release to get these changes in then, if I make time, will have a MINOR release to remove unneeded features

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

Successfully merging this pull request may close these issues.

3 participants