ioos / compliance-checker

Python tool to check your datasets against compliance standards
http://ioos.github.io/compliance-checker/
Apache License 2.0
104 stars 56 forks source link

removed last of pkg_resources. #1060

Closed ChrisBarker-NOAA closed 2 months ago

ChrisBarker-NOAA commented 2 months ago

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.

codecov[bot] commented 2 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ChrisBarker-NOAA commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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

ocefpaf commented 2 months ago

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.