Skip to content
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

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

Merged
merged 4 commits into from
Nov 16, 2019

Conversation

danb-pcs
Copy link
Contributor

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

…s, units, and space units. This feature was previously marked as 'TODO'.
@codecov-io
Copy link

codecov-io commented Jul 24, 2019

Codecov Report

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

Impacted file tree graph

@@            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%) ⬆️
nrrd/reader.py 100% <100%> (ø) ⬆️

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
Copy link
Collaborator

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
Copy link
Contributor Author

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

@addisonElliott
Copy link
Collaborator

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
Copy link
Contributor Author

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

@addisonElliott
Copy link
Collaborator

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

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

* Remove format_quoted_string_list, can format string lists with
  quotes using one line of code
* Remove parse_quoted_string_list, use Python-included shlex module for
  parsing
* Add tests for parsing quoted string lists (and ensuring that no quotes
  also works)
* Add tests for formatting quoted string lists for labels, units, and
  space units header fields
@addisonElliott addisonElliott merged commit 9acf9c4 into mhe:master Nov 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants