MNT: Drop distutils #3458
MNT: Drop distutils #3458
Conversation
effigies
force-pushed
the
mnt/drop_distutils_take2
branch
3 times, most recently
from
2952640 to
35d792c
Compare
Codecov Report
@@ Coverage Diff @@
## master #3458 +/- ##
=======================================
Coverage 65.24% 65.24%
=======================================
Files 309 309
Lines 40838 40827 -11
Branches 5377 5377
=======================================
- Hits 26645 26639 -6
+ Misses 13118 13114 -4
+ Partials 1075 1074 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Much needed, thanks for pushing on this.
One thing - packaging.version.LegacyVersion might be a better fit for a lot of the non-Python tools, since they may not follow PEP440 (especially if using dev versions [this has bitten me before when working with dev FreeSurfer versions]). Or alternatively, we can use packaging.version.parse to produce either classes depending on the version string.
| if path is None: | ||
| path = os.getenv(MRTRIX3_HOME, "/opt/mrtrix3") | ||
| else: | ||
| path = op.dirname(op.dirname(path)) | ||
|
|
||
| self.inputs.in_config = op.join( | ||
| path, | ||
| "src/dwi/tractography/connectomics/" "example_configs/fs_default.txt", | ||
| "src/dwi/tractography/connectomics/example_configs/fs_default.txt", |
There was a problem hiding this comment.
shouldn't this be path expanded?
| "src/dwi/tractography/connectomics/example_configs/fs_default.txt", | |
| "src", | |
| "dwi", | |
| "tractography", | |
| "connectomics", | |
| "example_configs", | |
| "fs_default.txt", |
There was a problem hiding this comment.
It can be, but I don't see much advantage to it. Windows interprets forward slashes as path separators, and this fits on one line.
nipype/utils/filemanip.py
Outdated
| @@ -28,6 +28,9 @@ | |||
|
|
|||
| related_filetype_sets = [(".hdr", ".img", ".mat"), (".nii", ".mat"), (".BRIK", ".HEAD")] | |||
|
|
|||
| # Keep anybody who depended on the old hack from breaking | |||
| which = shutil.which | |||
There was a problem hiding this comment.
just a note, the keyword args (which can also be positional) are different
There was a problem hiding this comment.
Yeah. I guess we should keep and deprecate the old one instead.
There was a problem hiding this comment.
Can't you wrap calls to shutil.which in n/u/filemanip.py#which and keep the old function signature alive ?
I'd expect users who want to use shutil.which to import it directly instead of using nipype as a proxy for it anyway. However, anyone using nipype's which with named arguments would get an error which is sad. It depends how popular you think these utilities are in client code. I know Clinica uses some and we are trying to move away from them to reduce coupling.
Or a solution in between would be start with a DeprecationWarning advising to port to shutil.which and later remove or proxy it?
|
I tried this for a bit... LegacyVersion has deprecation warnings. |
|
I see, looks like they are adamant on removing it. It's a pity we're losing all these non PEP440 compliant checks. |
|
Maybe we should consider externals.version.LooseVersion a long term solution rather than a bridge. If it's Python, use Version, if it's not, use LooseVersion unless a custom method is needed |
|
I think that sounds reasonable |
Now that I have understood the needs a bit more, I believe that's the best you can do with what's available to you right now. |
effigies
force-pushed
the
mnt/drop_distutils_take2
branch
from
35d792c to
a012839
Compare
effigies
force-pushed
the
mnt/drop_distutils_take2
branch
from
a012839 to
1acd3fa
Compare
|
Thanks for your input @ghisvail. I've decided to defer the deprecation of |
Summary
FreeSurfer interfaces have some fairly deep LooseVersion integration that I didn't want to touch right now, so I just vendored LooseVersion in externals.
Fixes #3439.
List of changes proposed in this PR (pull-request)