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

removed last of pkg_resources. #1060

Merged
merged 5 commits into from
Apr 13, 2024
Merged

Conversation

ChrisBarker-NOAA
Copy link
Contributor

I've removed the last of pkg_resources, replacing it with:

importlib.metadata

for Python >= 3.9

and the backport:

import importlib_metadata

for older versions.

I also added conditional import of: importlib.resources, so the backport will only be used if it's needed.

Both the backports are in the requirements files, as I know of no way to make a requirement python-version-specific.

NOTE: do we really need to keep older versions working? With this kind of utility, I'd think folks could user a newish Python.

Copy link

codecov bot commented Apr 12, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 82.18%. Comparing base (d53d227) to head (064aac9).
Report is 2 commits behind head on develop.

Files Patch % Lines
compliance_checker/suite.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1060   +/-   ##
========================================
  Coverage    82.18%   82.18%           
========================================
  Files           24       24           
  Lines         5171     5171           
  Branches      1242     1242           
========================================
  Hits          4250     4250           
  Misses         622      622           
  Partials       299      299           

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

compliance_checker/base.py Outdated Show resolved Hide resolved
compliance_checker/cf/util.py Outdated Show resolved Hide resolved
compliance_checker/suite.py Outdated Show resolved Hide resolved
compliance_checker/suite.py Outdated Show resolved Hide resolved
compliance_checker/suite.py Outdated Show resolved Hide resolved
@ChrisBarker-NOAA
Copy link
Contributor Author

I've gotten pulled away from this, and wont have any more time to look at it -- If you want to keep all the backports in for the imports, then there's only a few lines over what you already did -- probably easier to just do that on a new PR than update this one.

@ChrisBarker-NOAA
Copy link
Contributor Author

My thoughts:

Had we done this a couple years ago, I'd go with the backports all the way. but Python is solidly at 3.12 now -- and particularly for this kind of project (not a lib that's used in legacy code) I think focus on newer Python's makes more sense than locking in a backport that won't been needed for long.

If it was just me, I'd probably go to 3.10+ (or at least 3.9) now and be done with it :-)

At the least, I'd keep the importlib imports in a comment, to make the update in the future

@ocefpaf
Copy link
Member

ocefpaf commented Apr 13, 2024

@jcermauwedu you raised some concern on the minimum Python supported here. This PR still keeps it Ptyhon>=3.8. However, it is likely we will follow NEP-29 closer in the future. Would that work for your deployments?

@ocefpaf ocefpaf merged commit 9cba162 into ioos:develop Apr 13, 2024
24 of 25 checks passed
@ocefpaf
Copy link
Member

ocefpaf commented Apr 13, 2024

@benjwadams this was the last PR I wanted to get in before 5.1.1. We are good to go :shipit:

@ocefpaf
Copy link
Member

ocefpaf commented Apr 13, 2024

My thoughts:

Had we done this a couple years ago, I'd go with the backports all the way. but Python is solidly at 3.12 now -- and particularly for this kind of project (not a lib that's used in legacy code) I think focus on newer Python's makes more sense than locking in a backport that won't been needed for long.

If it was just me, I'd probably go to 3.10+ (or at least 3.9) now and be done with it :-)

At the least, I'd keep the importlib imports in a comment, to make the update in the future

I'd definitely go 3.10 as min but the user base here is much larger than we expected. Also, the pyupgrade pre-commit makes it super easy to fix anything that is deprecated and to adapt for new code when we change the min python, so bo need for future proofing by adding extra if-clause imports.

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.

2 participants