-
Notifications
You must be signed in to change notification settings - Fork 134
Sanitize "locator" value #830
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
base: master
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR sanitizes the "locator" value to avoid spaces and special characters that could cause issues with dcm2niix execution due to a known nipype bug. The changes implement a path sanitization utility that replaces problematic characters with underscores.
- Adds a
sanitize_path
function to clean paths by replacing unwanted characters with underscores - Applies sanitization to the locator value in the main workflow
- Includes comprehensive test coverage for both valid and invalid path scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
heudiconv/utils.py | Adds delete_chars and sanitize_path utility functions for path cleaning |
heudiconv/main.py | Applies sanitize_path to the locator value in the workflow |
heudiconv/tests/test_utils.py | Adds parametrized tests for the new sanitization functions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
lgr.warning( | ||
"%r %s contained problematic character(s), it " "was cleaned to be %r", | ||
path, | ||
descr, | ||
clean_path, | ||
) |
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.
The string concatenation with a space between two string literals is unnecessary. The strings should be combined into a single string for better readability: \"%r %s contained problematic character(s), it was cleaned to be %r\"
Copilot uses AI. Check for mistakes.
d3bcf81
to
e774c2d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #830 +/- ##
==========================================
+ Coverage 82.84% 82.92% +0.07%
==========================================
Files 42 42
Lines 4313 4333 +20
==========================================
+ Hits 3573 3593 +20
Misses 740 740 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e774c2d
to
afb7626
Compare
We observed the Study Description to contain spaces in it. Due to unresolved bug in nipype (nipy/nipype#3604) we must avoid spaces in the file names since then external execution of dcm2niix would fail. While at it, I decided to sanitize it more and replace all other "funny" characters with the special treatment by shells.
afb7626
to
16c72a1
Compare
Do you rely on locator values extracted from DICOMs StudyDescription, @nipy/team-heudiconv ? I am planing to merge/release within a week or so |
LGTM, I think we were taking care of the sanitization in our heuristics |
We observed the Study Description to contain spaces in it. Due to unresolved bug in nipype (nipy/nipype#3604) we must avoid spaces in the file names since then external execution of dcm2niix would fail. While at it, I decided to sanitize it more and replace all other "funny" characters with the special treatment by shells.