search

NCAS-CMS / cfunits

A Python interface to UNIDATA’s UDUNITS-2 library with CF extensions:
http://ncas-cms.github.io/cfunits
MIT License
11 stars 8 forks source link

Replace distutils with pkg_resources in __init__.py due to deprecation #44

Closed gareth-j closed 1 year ago

gareth-j commented 2 years ago

This fixes the deprecation warning that was shown due to the deprecation of distutils. This replaces the use of LooseVersion with pkg_resources.parse_version in __init__.py.

gareth-j commented 2 years ago

From what I understand pkg_resources is provided by setuptools that most users should have within their environment whereas packaging is a separate package. I did a quick test by creating a new virtual environment and pkg_resources was available after creation but packaging wasn't. I'm happy to modify this PR to use packaging though if that's chosen.

davidhassell commented 2 years ago

I'm always keen to reduce dependencies, so would be happy to use pkg_resources across the board. @sadielbartholomew, is that OK with you?

davidhassell commented 1 year ago

Hi - I'm keen to create 3.3.5 later this week, and in the absence of further thoughts, I shall merge this PR very soon ...

sadielbartholomew commented 1 year ago

Sorry all, I missed this due to distractions from a few conferences, I will take a look in a moment...

codecov-commenter commented 1 year ago

Codecov Report

Merging #44 (f0ea423) into master (1f2d086) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #44   +/-   ##
=======================================
  Coverage   81.10%   81.10%           
=======================================
  Files           2        2           
  Lines         677      677           
=======================================
  Hits          549      549           
  Misses        128      128
Flag Coverage Δ
unittests 81.10% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cfunits/__init__.py 76.48% <100.00%> (ø)

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

sadielbartholomew commented 1 year ago

Hi both, sorry again for the late response.

We have migrated away from LooseVersion in the other NCAS-CMS cf packages, but clearly not this one.

References are PRs NCAS-CMS/cf-python#324 and NCAS-CMS/cfdm#173, just to include quick links in this thread.

I have one question, which perhaps you and/or @sadielbartholomew could answer - which is "better" (whatever that means): pkg_resources.parse_version as used here, or packaging.version.Version as used in cf-python and cfdm.

When I opened NCAS-CMS/cf-python#324 I made some notes, anticipating I might forget the details on this. We are advised by the deprecation warning to use the latter:

DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.

so, both given that direct pointer and to be consistent with cf-python and cfdm which have already been fixed with regards to the deprecation, as David states he would also like, I think we should use that instead.

However, note a side-effect to this choice is that we need to add packaging>=20.0 as a new dependency with this choice, as done and noted on the PRs for cf and cfdm.

So overall: @gareth-j, thanks for raising the underlying issue here and your work so far on it. Would you mind converting this PR to use packaging.version.Version instead of pkg_resources.parse_version, for the reasons we've covered in our comments? You can use #324 as a template. You will also need to add packaging to requirements.txt. Sorry that we want an alternative approach, I guess if you'd opened an Issue to cover this first we could have worked this out before you put up the PR. It shouldn't be much work or take much time at all to convert, but if you'd rather not or don't have the time, either David or myself can step in to do this. Thanks.

sadielbartholomew commented 1 year ago

The linting check has failed but based on my comment above we can ignore that for now...

davidhassell commented 1 year ago

Hi Sadie - thank you for your careful review, and reminding me of the history - I support your suggestions.

sadielbartholomew commented 1 year ago

@gareth-j: in the interests of time, because we want to make a new release of this library ASAP, I have updated the PR in line with my previous comments. We wanted to include your PR and commit as a thanks for your work on this, hence my pushing new commits rather than closing this and doing a separate PR.

@davidhassell: please can you give this a quick review, then we can merge and prepare the new release.

Thanks both!

gareth-j commented 1 year ago

Sorry I missed your comments @sadielbartholomew! And thanks for merging my PR!