NF: nib-convert CLI tool #1113
NF: nib-convert CLI tool #1113
Conversation
|
Hello @effigies, Thank you for updating! Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, Comment last updated at 2022-06-13 17:14:28 UTC |
Codecov Report
@@ Coverage Diff @@
## master #1113 +/- ##
==========================================
+ Coverage 92.30% 92.32% +0.02%
==========================================
Files 100 101 +1
Lines 12342 12380 +38
Branches 2419 2423 +4
==========================================
+ Hits 11392 11430 +38
Misses 625 625
Partials 325 325
Continue to review full report at Codecov.
|
|
Can I interest anyone in a review? Want to get this in for 4.0 as it includes a fix. |
| convert.main([str(infile), str(outfile)]) | ||
|
|
||
| convert.main([str(infile), str(outfile), '--force']) | ||
| assert outfile.is_file() |
There was a problem hiding this comment.
Not too important, but is there a way to make sure that this is the file that was created in the forced run, and not the one that existed there before? You might worry that the force flag would somehow make the program do nothing.
nibabel/cmdline/convert.py
Outdated
| "a new image like `infile` will be created and converted to a type " | ||
| "matching the extension of `outfile`.") | ||
| p.add_argument("-f", "--force", action="store_true", | ||
| help="Ignore warnings if possible") |
There was a problem hiding this comment.
This also forces over-writing existing files. Might want to mention that here.
|
Thanks @arokem. I think I addressed both of your concerns. Please have another look when you get a minute. |
|
Yep. This looks good to me and I am +1 to merge. Does nibabel have a protocol about needing additional reviewers, or should I hit "merge"? |
|
Nope, one reviewer is enough, unless several people have been active in the discussion. (We have some docs here now: https://nipy.org/nibabel/devel/core_developer.html#reviewing) |
|
Got it! Thanks. I'll wait on that one remaining CI run and then hit the merge button. |
In #1089 we proposed:
Also found an error in NIfTI writing if
get_data_dtype(finalize=True)wasn't called prior toto_file_map().