MNT: Finish removing distutils and setuptools dependencies #1190
Conversation
Codecov ReportBase: 91.88% // Head: 92.17% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1190 +/- ##
==========================================
+ Coverage 91.88% 92.17% +0.29%
==========================================
Files 97 97
Lines 12317 12341 +24
Branches 2168 2535 +367
==========================================
+ Hits 11317 11375 +58
+ Misses 663 645 -18
+ Partials 337 321 -16
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
bb96b5d to
580b628
Compare
|
This is pretty straightforward, IMO, if anybody wants to do a review. Drive-bys welcome. Will merge at the end of the week if no reviews. |
nibabel/tests/test_testing.py
Outdated
| os.path.join(os.path.dirname(__file__), '..', 'tests', 'data') | ||
| ) | ||
| for subdir in ('nicom', 'gifti', 'externals'): | ||
| assert test_data(subdir) == os.path.join(data_path[:-10], subdir, 'tests', 'data') | ||
| assert str(test_data(subdir)) == os.path.join( | ||
| data_path.parent.parent, subdir, 'tests', 'data' |
There was a problem hiding this comment.
Is data_path a Path instance? If so, wouldn't it be more natural to use the / operator instead of os.path.join? Also, maybe the assertions could use the samefile method?
There was a problem hiding this comment.
I added fa5f920 to address this. LMK what you think.
There was a problem hiding this comment.
LGTM.
I might, at some point in the future, if you don't think it's a bad idea, make a refactoring PR with a bunch of more explicit variable naming suggestions, e.g., here my instinct would be to do something like:
expected = data_path.parent.parent / subdir / 'tests' / 'data'
assert str(test_data(subdir)) == expectedThere was a problem hiding this comment.
You're more than welcome to.
580b628 to
ad439f5
Compare
|
@ZviBaratz Thanks for the review. In addition to fa5f920, I also added ad439f5 to rename Figured I'd throw it in while I was here. |
distutilsis removed in Python 3.12. Looking at https://github.com/python/cpython/blob/3467991/Lib/distutils/ccompiler.py#L919-L956, it turns out the single place we were still importing it was redundant with checkingos.name.Setuptools has recommended against using
pkg_resourcesfor some time, and it seems thatimportlib.resourceshas settled on an API that has been stable since Python 3.9. We can use theimportlib_resourcesbackport for Python 3.8.