-
Notifications
You must be signed in to change notification settings - Fork 532
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
Fixed: Missing substring should raise ValueError not KeyError + try-except code cleanup #2691
Fixed: Missing substring should raise ValueError not KeyError + try-except code cleanup #2691
Conversation
- nipype issue# 2690
Codecov Report
@@ Coverage Diff @@
## master #2691 +/- ##
=========================================
- Coverage 67.61% 67.6% -0.01%
=========================================
Files 340 340
Lines 43143 43143
Branches 5349 5349
=========================================
- Hits 29170 29168 -2
+ Misses 13271 13265 -6
- Partials 702 710 +8
Continue to review full report at Codecov.
|
@kchawla-pi - thank you for the PR! |
nipype/utils/spm_docs.py
Outdated
@@ -59,5 +59,5 @@ def _strip_header(doc): | |||
except ValueError: | |||
index = len(doc) | |||
return doc[:index] | |||
except KeyError as e: | |||
except ValueError as e: |
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 this whole block might be better rewritten as
try:
index = doc.index(hdr)
except ValueError as e:
raise_from(IOError('This docstring was not generated by Nipype!\n'), e)
index += len(hdr)
index += 1
doc = doc[index:]
try:
index = doc.index(cruft)
except ValueError:
index = len(doc)
return doc[:index]
This helps make clear where the exception is expected, and avoids catching unexpected exceptions.
Hi @kchawla-pi. Do you have a few minutes to finish this one off? I think we're almost ready to go. We'll be releasing 1.1.3 next week, and this would be a great addition! |
Ok. |
…rity - Cleanup suggested by @effigies
Congrats on the upcoming release! :~) |
Fix for issue #2690