search

suyashkumar / dicom

⚡High Performance DICOM Medical Image Parser in Go.
MIT License
935 stars 136 forks source link

Introduce element-by-element Writer API, add SkipMetadataReadOnNewParserInit Parser Option, add ParseOptions to Parser API. #208

Closed kristianvalind closed 2 years ago

kristianvalind commented 3 years ago

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.

kristianvalind commented 3 years ago

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.

suyashkumar commented 3 years ago

Thanks for sending this! Had a couple comments for your consideration!

kristianvalind commented 3 years ago

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 writeDataset method.

kristianvalind commented 3 years ago

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.

kristianvalind commented 3 years ago

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.

kristianvalind commented 3 years ago

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

kristianvalind commented 3 years ago

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!

kristianvalind commented 2 years ago

Thank you for valuable comments and review as always! Let me know if anything else is needed before merging.

kristianvalind commented 2 years ago

Using debug.Log like you suggested seems like a more elegant way of achieving the same result.

suyashkumar commented 2 years ago

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. 😄