-
Notifications
You must be signed in to change notification settings - Fork 88
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
Add some robustness in meta file group reading #29
Conversation
- allow file meta group attributes in arbitrary order - allow missing information version
// unknown tag, do nothing | ||
// could be an unsupported or non-standard attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit, they're reserved
Values of all Tags (0002,xxxx) are reserved for use by this Standard and later versions of DICOM.
which mentioned below the table on http://dicom.nema.org/medical/dicom/current/output/chtml/part10/chapter_7.html#table_7.1-1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well yes, but with this comment I am keeping two possible cases in mind:
- the standard is extended to admit more file meta attributes, but the library does not know them yet;
- a pseudo-DICOM producer invents its own attribute within the same group.
In either case, nothing happens, but in the future we might be interested in logging it differently from the case below this one.
object/src/meta.rs
Outdated
@@ -372,7 +345,10 @@ impl FileMetaTableBuilder { | |||
.ok_or_else(|| Error::InvalidFormat)?; | |||
let information_version = self | |||
.information_version | |||
.ok_or_else(|| Error::InvalidFormat)?; | |||
.or_else(|| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unwrap_or_else
may be handy here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, thanks! For some reason my IDE did not suggest me that method.
Awesome work! I'm going to try this branch, will share the results soon |
I've just tested it, #28 is fixed in this pr |
to provide a default file meta information version
Thank you for testing, @ibaryshnikov! |
Resolves #28.
(00H, 01H)
)CC @ibaryshnikov