mhe / pynrrd

Simple pure-python module for reading and writing nrrd files.

https://pynrrd.readthedocs.io/

MIT License

116 stars 51 forks source link

Support writing and reading NRRD header quoted string lists for label… #98

Closed danb-pcs closed 5 years ago

danb-pcs commented 5 years ago

This feature was previously marked in the source code as 'TODO'.

codecov-io commented 5 years ago

Codecov Report

Merging #98 into master will increase coverage by <.01%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #98      +/-   ##
==========================================
+ Coverage   99.46%   99.47%   +<.01%     
==========================================
  Files           6        6              
  Lines         374      381       +7     
  Branches      119      123       +4     
==========================================
+ Hits          372      379       +7     
  Misses          1        1              
  Partials        1        1

Impacted Files	Coverage Δ
nrrd/writer.py	`98.36% <100%> (+0.02%)`	:arrow_up:
nrrd/reader.py	`100% <100%> (ø)`	:arrow_up:

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 9407b01...5b72c37. Read the comment docs.

addisonElliott commented 5 years ago

Hello, thanks for the PR. At a first glance, it looks good. Some unit tests would be a good thing to add.

~~On a more conceptual level, I'm not sure if making the three fields (labels, units, space units) always be a quoted string list is preferred.~~

Well, according to the NRRD spec, I suppose those three types should be surrounded by quotations (source)

danb-pcs commented 5 years ago

Thanks! I will take a look at adding unit tests.

addisonElliott commented 5 years ago

Also, I'm not sure if your code does this, but we probably want to remain backwards compatible in the sense that we should correctly parse the "quoted string list"'s correctly even if there are no quotes. In other words, fallback to just a string list if there are no quotes in the quoted string list.

danb-pcs commented 5 years ago

Yes, the code accepts non-quoted strings, so it is backward-compatible with existing files.

addisonElliott commented 5 years ago

@pcs-dan Just checking up on your progress, any luck with creating unit tests?

If you don't or won't have the time, I may be able to create some and finish this PR up in the next few weeks.

danb-pcs commented 5 years ago

@addisonElliott I am sorry about the delay, I haven't had any time. I will try to add the tests in the next week.

addisonElliott commented 5 years ago

It's no problem, I just wanted to make sure you were still up for the task. Take your time