Skip to content

DICOM: fix parsing of DateTime entry #2531

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.

Sign up for GitHub

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

Jump to bottom
Merged
jdtournier merged 2 commits into from
Dec 7, 2022
Merged

DICOM: fix parsing of DateTime entry #2531

jdtournier merged 2 commits into from
Dec 7, 2022

Conversation

@jdtournier
Copy link
Member

@jdtournier jdtournier commented Dec 6, 2022

I noticed a few issues when parsing this entry, which basically boiled down to assuming the fractional part was always 6 digits long - which doesn't hold, it can be any length up to 6 digits...

Reference docs - look for entry DT DateTime

I'll run my DICOM tests before merging.

@jdtournier jdtournier added this to the 3.0.4 milestone Dec 6, 2022
@jdtournier jdtournier requested a review from a team December 6, 2022 12:08
@jdtournier jdtournier self-assigned this Dec 6, 2022
Lestropie
Lestropie approved these changes Dec 7, 2022
View reviewed changes
Copy link
Member

@Lestropie Lestropie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follows #2466. I don't think I've come across this one given you only implemented it 8 months ago, but it makes sense.

Will require merge of #2532 to pass CI.

@jdtournier jdtournier enabled auto-merge December 7, 2022 11:31
@jdtournier jdtournier merged commit ed69e28 into master Dec 7, 2022
@jdtournier jdtournier deleted the dicom_fix_datetime branch December 7, 2022 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lestropie Lestropie Lestropie approved these changes

Assignees

@jdtournier jdtournier

Labels

bug DICOM

Projects

None yet

Milestone

3.0.4

Development

Successfully merging this pull request may close these issues.

3 participants

@jdtournier @Lestropie