Skip to content
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

[FIX] Escape metacharacters when parsing dcm2niix outputs #3417

Merged
merged 4 commits into from
Dec 8, 2021

Conversation

qian587
Copy link

@qian587 qian587 commented Nov 29, 2021

Summary

This PR should fix the issue with dcm2niix interface not able to parse the output files when the file name contains meta characters.

Fixes #3416 .

List of changes proposed in this PR (pull-request)

Acknowledgment

  • (Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

@effigies
Copy link
Member

This looks reasonable, though I'd suggest escaping just before the glob, rather than passing around an escaped filename:

 def search_files(prefix, outtypes):
-    return it.chain.from_iterable(iglob(prefix + outtype) for outtype in outtypes)
+    return it.chain.from_iterable(iglob(glob.escape(prefix + outtype))
+                                  for outtype in outtypes)

@effigies
Copy link
Member

Would you mind writing a small test for search_files() that validates this behavior, by the way?

@qian587
Copy link
Author

qian587 commented Nov 29, 2021

Yea for sure, unsure where exactly I should be putting the escape in the first place. Will modify the code and add a testing too. Thanks 🙏

@qian587
Copy link
Author

qian587 commented Nov 29, 2021

@effigies May I know where should I put the test for search_files() ?

@effigies
Copy link
Member

We put tests for nipype/subdir/x.py in nipype/subdir/tests/test_x.py, so in this case it would be nipype/interfaces/tests/test_dcm2nii.py.

@codecov
Copy link

codecov bot commented Nov 29, 2021

Codecov Report

Merging #3417 (09ff390) into master (f756b71) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3417      +/-   ##
==========================================
+ Coverage   65.21%   65.23%   +0.02%     
==========================================
  Files         307      307              
  Lines       40474    40538      +64     
  Branches     5351     5370      +19     
==========================================
+ Hits        26395    26446      +51     
- Misses      13004    13016      +12     
- Partials     1075     1076       +1     
Flag Coverage Δ
unittests 64.95% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
nipype/interfaces/dcm2nii.py 65.27% <100.00%> (+0.16%) ⬆️
nipype/interfaces/dipy/base.py 76.70% <0.00%> (+1.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f756b71...09ff390. Read the comment docs.

@qian587
Copy link
Author

qian587 commented Dec 6, 2021

@effigies I have added the test for search_files() let me know what you think. Thank you

@effigies effigies changed the title [FIX] added escape for metacharacters [FIX] Escape metacharacters when parsing dcm2niix outputs Dec 8, 2021
@effigies
Copy link
Member

effigies commented Dec 8, 2021

LGTM. Let's make sure the tests pass...

@effigies effigies merged commit efc0702 into nipy:master Dec 8, 2021
@effigies effigies added this to the 1.7.1 milestone Apr 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty output files list if filename contains metacharacters in dcm2nii
2 participants