-
Notifications
You must be signed in to change notification settings - Fork 84
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
Revamp DICOM datetime #453
Conversation
- deprecation messages - add basic D/T values to prelude - clippy fixes
- default handling of ambiguous DT Ranges - needs chrono 'clock' feature
Hello, @Enet4. Please check out this pull request. My initial questions: The Also, for different parsing behavior of this situation a I'm not sure if I deprecated some methods correctly ... Thanks |
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 very much for working on this, @jmlaka. I haven't had much time to review things properly, but it would be nice to bring a new version of DICOM-rs the next few months, so I'm aiming for that.
Maybe the [PreciseDateTimeResult] struct name is too long ?
It should be OK. The only alternative I can think of is PreciseDtResult, but that would reduce the size of the most important part of the type in the name.
The core crate now requires the "clock" feature for chrono library, I don't know if this is acceptable.It's needed for the default handling of an ambiguous DT range, where one time zone is parsed and the second one is missing.
Interesting, I did not recall that we were excluding some of the default features. As long as it still builds against major platforms and wasm32, it should be fine. If not, we could try some feature gating.
Also, for different parsing behavior of this situation a PrimitiveValue:to_datetime_range_custom was added. I'm not sure if you are ok with such addition.
At first glance the name to_datetime_range_custom
is not very clear about what custom
entails, but considering that it's a custom interpretation (or disambiguation) of the range, I don't see a better name for it. I will strive for a more thorough review on this part next week or so.
I'm not sure if I deprecated some methods correctly
I left a note on that in the inline comments.
- fix doctest
I fixed the problems you mentioned. |
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.
Things look pretty good! I just have a few more changes to suggest, and then we can merge.
- fix doc links
- clarify its purpose in the documentation - document variants - rename existing methods and add more methods
- do not provide an order between non-matching variants
I hope you don't mind, I added in a few more changes before merging. Here is the summary:
With this, I think we are ready to move forward. :) |
Thank you very much for your added insight. |
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 once again. 👍 We're bringing this in.
Please see this DicomDateTime rewrite that now can handle time zone aware and naive values.
External FixedOffset values no longer required.
some methods deprecated
some methods documented to be not optimal use of API
Breaking changes
This should fix #452
Thank you !