Skip to content

Conversation

@Cryt1c
Copy link
Contributor

@Cryt1c Cryt1c commented Oct 4, 2025

Implementation of #691

  1. Instead of using "today" for the date I decided to use "now" for all methods. Please let me know if you still wanna go for today.
  2. I created a variant for the local time and for utc.
  3. I am not sure how I should setup the tests here. Should I look into mocking the chronos library or do you have another idea on how to handle it?

Looking forward to your feedback!

@Enet4 Enet4 added A-lib Area: library C-core Crate: dicom-core labels Oct 4, 2025
@Cryt1c Cryt1c changed the title Add DicomDate::now, DicomDate::now and DicomDateTime::now Add now_local and now_utc for DicomDate, DicomDate and DicomDateTime Oct 4, 2025
@Cryt1c Cryt1c marked this pull request as ready for review October 4, 2025 11:29
Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

This is much appreciated!

  1. Instead of using "today" for the date I decided to use "now" for all methods. Please let me know if you still wanna go for today.

OK, I just looked and even the today methods in chrono are deprecated, so we'd better not use them from the start!

  1. I created a variant for the local time and for utc.

Makes sense. DICOM is particularly quirky here, because many key attributes are split by date and time, and the timezone cannot be written when this happens. There may be even more ways to assist users when working with DICOM dates and times, but offering these possibilities should be a step in the right direction.

  1. I am not sure how I should setup the tests here. Should I look into mocking the chronos library or do you have another idea on how to handle it?

I think it's OK that we test this by issuing a now date-time via chrono to serve as a reference. Then for the date-time output we would check for differences with the reference so that they are within an acceptable margin (like 1 second or so).

@Cryt1c Cryt1c requested a review from Enet4 October 15, 2025 18:58
Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

Thank you!

@Enet4 Enet4 merged commit 22507e3 into Enet4:master Nov 14, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-lib Area: library C-core Crate: dicom-core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants