fix: addressed @effigies' comments:

- [x] Removed traits.api patches
- [x] Deduplicated code with a _recurse_on_path_traits proxy
      (nipy#2970 (comment))
- [x] Added the two proposed test cases
- [x] Optimized loop (see nipy#2970 (comment))

Author: oesteban

nipype/interfaces/base/__init__.py

@@ -20,8 +20,9 @@
                     StdOutCommandLineInputSpec)
 
 from .traits_extension import (
-    traits, Undefined, isdefined,
-    File, Directory, Str, DictStrStr, has_metadata, ImageFile,
+    traits, Undefined, isdefined, has_metadata,
+    File, ImageFile, Directory,
+    Tuple, Either, Str, DictStrStr,
     OutputMultiObject, InputMultiObject,
     OutputMultiPath, InputMultiPath)
 

nipype/interfaces/base/tests/test_traits_extension.py

@@ -8,14 +8,13 @@
 
 
 class _test_spec(nib.TraitedSpec):
-    a = nib.traits.File()
-    b = nib.traits.Tuple(nib.File(),
-                         nib.File())
+    a = nib.File()
+    b = nib.Tuple(nib.File(), nib.File())
     c = nib.traits.List(nib.File())
-    d = nib.traits.Either(nib.File(), nib.traits.Float())
+    d = nib.Either(nib.File(), nib.traits.Float())
     e = nib.OutputMultiObject(nib.File())
     f = nib.traits.Dict(nib.Str, nib.File())
-    g = nib.traits.Either(nib.File, nib.Str)
+    g = nib.Either(nib.File, nib.Str)
     h = nib.Str
     ee = nib.OutputMultiObject(nib.Str)
 
@@ -28,6 +27,10 @@ def test_rebase_path_traits():
         spec.trait('a'), '/some/path/f1.txt', '/some/path')
     assert a == Path('f1.txt')
 
+    a = rebase_path_traits(
+        spec.trait('a'), '/some/path/f1.txt', '/some/other/path')
+    assert a == Path('/some/path/f1.txt')
+
     b = rebase_path_traits(
         spec.trait('b'), ('/some/path/f1.txt', '/some/path/f2.txt'), '/some/path')
     assert b == (Path('f1.txt'), Path('f2.txt'))
@@ -90,6 +93,10 @@ def test_resolve_path_traits():
         spec.trait('a'), 'f1.txt', '/some/path')
     assert a == Path('/some/path/f1.txt')
 
+    a = resolve_path_traits(
+        spec.trait('a'), '/already/absolute/f1.txt', '/some/path')
+    assert a == Path('/already/absolute/f1.txt')
+
     b = resolve_path_traits(
         spec.trait('b'), ('f1.txt', 'f2.txt'), '/some/path')
     assert b == (Path('/some/path/f1.txt'), Path('/some/path/f2.txt'))
@@ -128,10 +135,10 @@ def test_resolve_path_traits():
 
     # This is a problematic case, it is impossible to know whether this
     # was meant to be a string or a file.
-    # Commented out because in this implementation, strings take precedence
-    # g = resolve_path_traits(
-    #     spec.trait('g'), 'path/either.txt', '/some')
-    # assert g == Path('/some/path/either.txt')
+    # In this implementation, strings take precedence
+    g = resolve_path_traits(
+        spec.trait('g'), 'path/either.txt', '/some')
+    assert g == 'path/either.txt'
 
     # This is a problematic case, it is impossible to know whether this
     # was meant to be a string or a file.

nipype/interfaces/base/traits_extension.py

@@ -308,11 +308,6 @@ def validate(self, objekt, name, value, return_pathlike=False):
         return value
 
 
-# Patch in traits these two new
-traits.File = File
-traits.Directory = Directory
-
-
 class ImageFile(File):
     """Defines a trait whose value must be a known neuroimaging file."""
 
@@ -489,7 +484,7 @@ def inner_traits(self):
         return self.types
 
 
-class PatchedEither(TraitType):
+class Either(TraitType):
     """Defines a trait whose value can be any of of a specified list of traits."""
 
     def __init__(self, *traits, **metadata):
@@ -504,7 +499,7 @@ def as_ctrait(self):
 
 
 traits.Tuple = Tuple
-traits.Either = PatchedEither
+traits.Either = Either
 
 
 def _rebase_path(value, cwd):
@@ -523,34 +518,6 @@ def _rebase_path(value, cwd):
     return value
 
 
-def rebase_path_traits(thistrait, value, cwd):
-    """Rebase a BasePath-derived trait given an interface spec."""
-    if thistrait.is_trait_type(BasePath):
-        value = _rebase_path(value, cwd)
-    elif thistrait.is_trait_type(traits.List):
-        innertrait, = thistrait.inner_traits
-        if not isinstance(value, (list, tuple)):
-            value = rebase_path_traits(innertrait, value, cwd)
-        else:
-            value = [rebase_path_traits(innertrait, v, cwd)
-                     for v in value]
-    elif thistrait.is_trait_type(traits.Dict):
-        _, innertrait = thistrait.inner_traits
-        value = {k: rebase_path_traits(innertrait, v, cwd)
-                 for k, v in value.items()}
-    elif thistrait.is_trait_type(Tuple):
-        value = tuple([rebase_path_traits(subtrait, v, cwd)
-                       for subtrait, v in zip(thistrait.inner_traits, value)])
-    elif thistrait.alternatives:
-        is_str = [f.is_trait_type((traits.String, traits.BaseStr, traits.BaseBytes, Str))
-                  for f in thistrait.alternatives]
-        if any(is_str) and isinstance(value, (bytes, str)) and not value.startswith('/'):
-            return value
-        for subtrait in thistrait.alternatives:
-            value = rebase_path_traits(subtrait, value, cwd)
-    return value
-
-
 def _resolve_path(value, cwd):
     if isinstance(value, list):
         return [_resolve_path(v, cwd) for v in value]
@@ -565,29 +532,41 @@ def _resolve_path(value, cwd):
     return value
 
 
-def resolve_path_traits(thistrait, value, cwd):
-    """Resolve a BasePath-derived trait given an interface spec."""
+def _recurse_on_path_traits(func, thistrait, value, cwd):
+    """Run func recursively on BasePath-derived traits."""
     if thistrait.is_trait_type(BasePath):
-        value = _resolve_path(value, cwd)
+        value = func(value, cwd)
     elif thistrait.is_trait_type(traits.List):
         innertrait, = thistrait.inner_traits
         if not isinstance(value, (list, tuple)):
-            value = resolve_path_traits(innertrait, value, cwd)
+            value = _recurse_on_path_traits(func, innertrait, value, cwd)
         else:
-            value = [resolve_path_traits(innertrait, v, cwd)
+            value = [_recurse_on_path_traits(func, innertrait, v, cwd)
                      for v in value]
     elif thistrait.is_trait_type(traits.Dict):
         _, innertrait = thistrait.inner_traits
-        value = {k: resolve_path_traits(innertrait, v, cwd)
+        value = {k: _recurse_on_path_traits(func, innertrait, v, cwd)
                  for k, v in value.items()}
     elif thistrait.is_trait_type(Tuple):
-        value = tuple([resolve_path_traits(subtrait, v, cwd)
+        value = tuple([_recurse_on_path_traits(func, subtrait, v, cwd)
                        for subtrait, v in zip(thistrait.inner_traits, value)])
     elif thistrait.alternatives:
         is_str = [f.is_trait_type((traits.String, traits.BaseStr, traits.BaseBytes, Str))
                   for f in thistrait.alternatives]
         if any(is_str) and isinstance(value, (bytes, str)) and not value.startswith('/'):
             return value
-        for subtrait in thistrait.alternatives:
-            value = resolve_path_traits(subtrait, value, cwd)
+        is_basepath = [f.is_trait_type(BasePath) for f in thistrait.alternatives]
+        if any(is_basepath):
+            subtrait = thistrait.alternatives[is_basepath.index(True)]
+            value = _recurse_on_path_traits(func, subtrait, value, cwd)
     return value
+
+
+def rebase_path_traits(thistrait, value, cwd):
+    """Rebase a BasePath-derived trait given an interface spec."""
+    return _recurse_on_path_traits(_rebase_path, thistrait, value, cwd)
+
+
+def resolve_path_traits(thistrait, value, cwd):
+    """Resolve a BasePath-derived trait given an interface spec."""
+    return _recurse_on_path_traits(_resolve_path, thistrait, value, cwd)