-
Notifications
You must be signed in to change notification settings - Fork 105
Add now_local and now_utc for DicomDate, DicomDate and DicomDateTime #700
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
Conversation
Enet4
left a comment
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.
This is much appreciated!
- 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!
- 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.
- 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).
Enet4
left a comment
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.
Thank you!
Implementation of #691
Looking forward to your feedback!