-
Notifications
You must be signed in to change notification settings - Fork 536
enh: selectfiles select(only)dirs too #2134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2134 +/- ##
==========================================
- Coverage 72.27% 72.27% -0.01%
==========================================
Files 1174 1174
Lines 58738 58741 +3
Branches 8454 8455 +1
==========================================
+ Hits 42453 42454 +1
- Misses 14924 14925 +1
- Partials 1361 1362 +1
Continue to review full report at Codecov.
|
|
could you please add a test with some fake directories and ensure that the test covers scenarios where there is and is not an os.sep at the end of the template quick question: why couldn't the user at the separator at the end? is this addition necessary? |
|
on 20s of further thought, i'm not sure this is change is essential, as a user can easily add the separator, no? |
|
@satra the separator will be removed since |
|
@mgxd Maybe just check |
|
@effigies I like that better than my initial solution |
effigies
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Couple style comments.
nipype/interfaces/io.py
Outdated
|
|
||
| find_dirs = False | ||
| if template[-1] == os.sep: | ||
| find_dirs = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find_dirs = template[-1] == os.sep
nipype/interfaces/io.py
Outdated
| desc=("Whether to return outputs as a list even when only one file " | ||
| "matches the template. Either a boolean that applies to all " | ||
| "output fields or a list of output field names to coerce to " | ||
| " a list")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these changes should be reverted, or the arguments on the first line of each trait should also be dropped to a newline and un-indented.
|
GTK-thanks! |
Fixes #2132 .
Changes proposed in this pull request