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

Remove deprecated call to pkg_resources #36

Merged
merged 10 commits into from
Aug 11, 2023

Conversation

jfoster17
Copy link
Member

@jfoster17 jfoster17 commented Apr 3, 2023

Remove explicit calls to pkg_resources

Description

Sort of following the process in glue-viz/glue#2365, this updates echo to not depend on the deprecated pkg_resources API, at the cost of requiring importlib_metadata for older versions of Python (but glue installations will have this dependency anyway with the above-referenced change).

@jfoster17 jfoster17 requested a review from astrofrog April 3, 2023 19:17
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.09% 🎉

Comparison is base (ab38456) 96.65% compared to head (133a262) 96.74%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #36      +/-   ##
==========================================
+ Coverage   96.65%   96.74%   +0.09%     
==========================================
  Files          17       17              
  Lines        2182     2182              
==========================================
+ Hits         2109     2111       +2     
+ Misses         73       71       -2     
Files Changed Coverage Δ
echo/version.py 100.00% <100.00%> (+33.33%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jfoster17
Copy link
Member Author

I'm not sure exactly what's going on what that remaining test failure.

@dhomeier
Copy link
Contributor

dhomeier commented Apr 3, 2023

OSError: [WinError 123] The filename, directory name, or volume label syntax is incorrect: '--cov-report=xml:D:\a\echo\echo\coverage.xml'

I recall seeing that elsewhere with an older Windows env, obviously messing up the volume label in the default --cov-report=xml:{toxinidir}/coverage.xml or whatever it is; don't remember how that was fixed though (other than retiring Python 3.7 at least for the Windows test perhaps) – pyqt510 is quite old...

@astrofrog
Copy link
Member

Thanks! We've already dropped Python 3.7 for glue-core so I think we can just require >=3.8 here now?

@dhomeier
Copy link
Contributor

dhomeier commented Apr 4, 2023

Was wondering if it was required for pyqt510, but 5.10.1 seems to have wheels for 3.8 already.

@jfoster17
Copy link
Member Author

Was wondering if it was required for pyqt510, but 5.10.1 seems to have wheels for 3.8 already.

Looks like I pushed just as you were commenting here. I removed the py37 tests, but do you want one back for pyqt510? The main glue package requires >=5.14, so we could just drop some older versions...

@dhomeier
Copy link
Contributor

dhomeier commented Apr 4, 2023

Was just wondering if there was a specific reason to test the older versions here, but otherwise yes, keeping them in sync with glue-core (which underwent a major cleanup last year) probably makes sense.

@astrofrog
Copy link
Member

Keeping in sync with glue-core is fine!

Copy link
Contributor

@dhomeier dhomeier left a comment

Choose a reason for hiding this comment

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

Added some suggestions for py311 test envs, will still need to be added to tox.ini – @jfoster17 we should re-run the CI with these, and then this should finally be ready to go.

.github/workflows/ci_workflows.yml Show resolved Hide resolved
.github/workflows/ci_workflows.yml Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
@dhomeier dhomeier added the bug label Aug 5, 2023
jfoster17 and others added 4 commits August 11, 2023 15:23
Co-authored-by: Derek Homeier <[email protected]>
Co-authored-by: Derek Homeier <[email protected]>
Co-authored-by: Derek Homeier <[email protected]>
@dhomeier
Copy link
Contributor

Thanks, everything looking great! I will probably squash for the merge.

@jfoster17
Copy link
Member Author

Okay, please do squash.

@dhomeier
Copy link
Contributor

Alternatively, if you could squash just the last 4 commits into one and force-push, should be good as well. But either way is fine.

@dhomeier dhomeier merged commit 9747254 into glue-viz:main Aug 11, 2023
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants