ENH: Add versioning metadata to crash files #2626
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2626 +/- ##
==========================================
- Coverage 67.6% 67.56% -0.04%
==========================================
Files 339 339
Lines 42820 43372 +552
Branches 5290 5339 +49
==========================================
+ Hits 28948 29306 +358
- Misses 13189 13337 +148
- Partials 683 729 +46
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for the PR @anibalsolon - I've left a little comments, this looks handy! but perhaps @satra or @effigies can give this a quick review as well.
nipype/utils/filemanip.py
Outdated
| if versioning: | ||
| from nipype import __version__ as version | ||
| metadata = json.dumps({'version': version}, | ||
| ensure_ascii=True, |
There was a problem hiding this comment.
why not use UTF-8 like we do throughout the package?
There was a problem hiding this comment.
My initial preference around ASCII over UTF-8, in this case, is about Python 2 retro-compatibility.
If the error contains a path, for example, that uses an "é", Python 2 might not get so happy about it. Since JSON can be fully encoded using ASCII, I thought that this config might avoid some headaches.
I've done some tests now, and using UTF-8 will not be a problem at all.
nipype/utils/filemanip.py
Outdated
| encoding='ascii') | ||
|
|
||
| pkl_file.write(metadata.encode('ascii')) | ||
| pkl_file.write('\n'.encode('ascii')) |
| @@ -643,11 +643,44 @@ def loadpkl(infile): | |||
| else: | |||
| pkl_file = open(infile, 'rb') | |||
|
|
|||
| if versioning: | |||
| pkl_metadata = {} | |||
|
|
|||
There was a problem hiding this comment.
many blank lines (personal preference, feel free to ignore)
|
Thank you for the review @mgxd ! |
Fixes #2625.
Changes proposed in this pull request