Introduce element-by-element Writer API, add SkipMetadataReadOnNewParserInit Parser Option, add ParseOptions to Parser API. #208
Conversation
|
I've added the ability to set transfer syntax for a writer, which is useful for writing snippets without headers. For parsing snippets without headers, I've added the ParseOption type (analogous to WriteOption), and the SkipHeaderRead() ParseOption. |
|
Thanks for sending this! Had a couple comments for your consideration! |
|
Thank you so much for reviewing and for your comments! I agree that having only one way to write a dataset to file is the preferable way, and so I've unexported the |
|
I noticed that I needed to set the transfer syntax when reading as well as when writing. I've added a Parser.SetTransferSyntax method equivalent to the Writer.SetTransferSyntax. |
There was a problem hiding this comment.
Overall I think this is looking good, just a few more minor comments + test thoughts! Ultimately I think we can boil down this change to the following (summarizing for myself and future readers):
- No backwards-incompatible API changes or deletions for all existing APIs
- Introduce optional ParseOption (same pattern as existing WriteOption), and add initial SkipHeaderRead option
- Add Parser.SetTransferSyntax to allow callers to change the transfer syntax
- Add a Writer struct in the same pattern as the Parser struct
Thanks @kristianvalind!
|
Thanks again! I made one more minor change to NewParser, so that we don't try to read and parse a transfer syntax if the header read was skipped. I will look into writing a few tests for NewParser and the Writer structs, probably within the next two weeks or so. |
|
I've added one test each for Writer with WriteElement and NewParser with SkipHeaderRead. Let me know if they (or something else) should be changed in any way |
There was a problem hiding this comment.
Thanks again @kristianvalind! One more round of thoughts for you, but I think we're pretty close here!
parse_test.go
Outdated
| @@ -45,6 +45,30 @@ func TestParse(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| // TestNewParserSkipHeaderRead tests that NewParser with the SkipHeaderRead option | |||
| // parses the specified dataset but not its header. | |||
There was a problem hiding this comment.
Note, in practice this way of skipping the header won't actually skip the header data in the dicom if it's present (it'll just stop the parser from running the metadata read section of the code).
Maybe we should make that more clear in the option?
That is, if I pass a dicom that has header data into a Parser initialized with SkipHeaderRead, whenever we start reading elements from that parser, we will still get Metadata elements in our normal parse process. In otherwords, we don't advance the internal reader to skip metadata elements if there are any in the dicom, right?
I think maybe this could be a point of confusion for a user for an option entitled SkipHeaderRead. One option is that we read the metadata elements (if any) but throw them all away. Or, perhaps we should document this option more carefully--maybe call it something like SkipMetadataReadOnNewParserInit or something to make it clear that all the option does is prevent the NewParser() from trying to read metadata. But if metadata is actually present in the dicom, it's not like we skip parsing those elements (we should mention this in the comment regardless of what we call it).
Let me know if this makes sense! Apologies for the back and forth--the test is useful for seeing things in this framing.
write_test.go
Outdated
| @@ -667,3 +668,49 @@ func setUndefinedLength(e *Element) *Element { | |||
| e.ValueLength = tag.VLUndefinedLength | |||
| return e | |||
| } | |||
|
|
|||
| // TestWriteElement tests that Write and Writer.WriteElement produces identical results. | |||
There was a problem hiding this comment.
perhaps a better test may be something similar to the Write test now--that is lets write out a Dataset using Writer.WriteElement, then read it back in with Parser and check if they're identical?
I mention this because L686-L707 here is making an internal assumption about what Write does, when maybe we shouldn't in order to test the behavior of WriteElement. e.g. what if Write changes in the future to add a special metadata element in all things written out by the parser? It would seem weird to have to modify the test for something meant to test WriteElement in that case.
This is somewhat true in testing it against Parse, except we know Parse should always produce a consistent dataset with whatever stream of elements was written with WriteElement.
I guess this is a minor point, so please let me know what you think.
There was a problem hiding this comment.
We can also address this in a separate PR if we want, but the other comment/discussion is the only main blocker to merging imo.
|
I've renamed SkipHeaderRead to SkipMetadataReadOnNewParserInit, which imo is a lot more descriptive of what it actually does. I've also rewritten TestWriteElement so that it does not make any assumptions about Write. There may still be room for improvement, let me know what you think! |
There was a problem hiding this comment.
Fantastic, thanks @kristianvalind! This is looking pretty good. I think we're actually down to the last final nits and pretty much ready to merge.
I can also merge this now and address the final comments in a follow up myself if you are busy or prefer! Thanks again for the contribution.
parse.go
Outdated
| if optSet.skipMetadataReadOnNewParserInit { | ||
| p.reader.SetTransferSyntax(bo, implicit) | ||
|
|
||
| return &p, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Instead of setting this explicitly, can we let the following logic run below, which will ultimately end up setting the transfer syntax to the same setting?
Even though special casing will save a lookup, I prefer to avoid adding special cases unless they aid readability a lot.
(Let me know if I misunderstood)
There was a problem hiding this comment.
The reason I included this check was mostly to skip the warnings from the logic below, since they will always fire when we're not reading metadata from the dataset. Another option would be to check for optSet.skipMetadataReadOnNewParserInit before emitting the warnings. I'm not sure which is best. There might also be other, better ways.
There was a problem hiding this comment.
I see! If log spam is the biggest concern, how about if we switch these to debug.Log (so they will only be logged if the binary is build with a debug build tag) [they probably been switched over to debug.Log anyway)]?
I don't feel too strongly, so we can leave it as is, but let me know what you think!
|
Thank you for valuable comments and review as always! Let me know if anything else is needed before merging. |
parse.go
Outdated
| if optSet.skipMetadataReadOnNewParserInit { | ||
| p.reader.SetTransferSyntax(bo, implicit) | ||
|
|
||
| return &p, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
I see! If log spam is the biggest concern, how about if we switch these to debug.Log (so they will only be logged if the binary is build with a debug build tag) [they probably been switched over to debug.Log anyway)]?
I don't feel too strongly, so we can leave it as is, but let me know what you think!
Co-authored-by: Suyash Kumar <suyashkumar2003@gmail.com>
|
Using debug.Log like you suggested seems like a more elegant way of achieving the same result. |
suyashkumar
changed the title
|
Merged 🎉 !! Thanks @kristianvalind for the very useful contribution, and thanks for using the library! Please do continue to post any issues you have, send PRs, or help with any of the outstanding issues if you're interested. 😄 |
This is my proposal for a Writer struct to make single element writing available to other packages, as discussed in #206. It consists of a new Writer struct, with WriteDataset and and WriteElement methods, for whole dataset and single element writing respectively.