Skip to content
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

Merged
merged 23 commits into from
Mar 7, 2024
Merged

Revamp DICOM datetime #453

merged 23 commits into from
Mar 7, 2024

Conversation

jmlaka
Copy link
Contributor

@jmlaka jmlaka commented Feb 8, 2024

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 !

@jmlaka jmlaka marked this pull request as draft February 10, 2024 16:25
@jmlaka jmlaka marked this pull request as ready for review February 13, 2024 14:30
@jmlaka
Copy link
Contributor Author

jmlaka commented Feb 15, 2024

Hello, @Enet4. Please check out this pull request.

My initial questions:
Maybe the [PreciseDateTimeResult] struct name is too long ?

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.

Also, for different parsing behavior of this situation a PrimitiveValue:to_datetime_range_custom<T> was added. I'm not sure if you are ok with such addition.

I'm not sure if I deprecated some methods correctly ...

Thanks

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 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.

core/src/value/deserialize.rs Outdated Show resolved Hide resolved
core/src/value/partial.rs Outdated Show resolved Hide resolved
core/src/value/partial.rs Outdated Show resolved Hide resolved
core/src/value/partial.rs Outdated Show resolved Hide resolved
object/src/mem.rs Outdated Show resolved Hide resolved
core/src/value/primitive.rs Outdated Show resolved Hide resolved
@jmlaka
Copy link
Contributor Author

jmlaka commented Feb 21, 2024

I fixed the problems you mentioned.
The DicomDateTime had methods, which, on second thought, were superfluous.

@Enet4 Enet4 added bug This is a bug breaking change Hint that this may require a major version bump on release A-lib Area: library C-core Crate: dicom-core labels Feb 23, 2024
@Enet4 Enet4 self-requested a review February 23, 2024 11:23
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.

Things look pretty good! I just have a few more changes to suggest, and then we can merge.

core/src/value/range.rs Outdated Show resolved Hide resolved
core/src/value/partial.rs Outdated Show resolved Hide resolved
core/src/value/partial.rs Outdated Show resolved Hide resolved
object/src/mem.rs Outdated Show resolved Hide resolved
core/src/value/mod.rs Outdated Show resolved Hide resolved
Enet4 added 3 commits March 1, 2024 18:00
- clarify its purpose in the documentation
- document variants
- rename existing methods and add more methods
- do not provide an order between non-matching variants
@Enet4
Copy link
Owner

Enet4 commented Mar 2, 2024

I hope you don't mind, I added in a few more changes before merging. Here is the summary:

  • Adjusted the documentation around parse_datetime_partial, parse_datetime, PreciseDateTime, and the ambiguous DT parsers.
  • Readjusted the methods in PreciseDateTime to better reflect its nature as a sum type of chrono date-time values.
  • Made a custom PartialOrd implementation for PreciseDateTime because the derived one would be misleading. Because of how PartialOrd is derived, it could occur 2024-03-01 00:00:00 +00 being larger than 2024-03-01 23:59:59, much to the surprise of its users, just because it had a time zone specified.
  • Moved PreciseDateTime to live alongside the other DICOM date/time types.
  • Added impl FromStr for DicomDateTime.

With this, I think we are ready to move forward. :)

@Enet4 Enet4 changed the title Datetimefix Revamp DICOM datetime Mar 2, 2024
@jmlaka
Copy link
Contributor Author

jmlaka commented Mar 2, 2024

Thank you very much for your added insight.
Happy to help.

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 once again. 👍 We're bringing this in.

@Enet4 Enet4 merged commit 070ebe9 into Enet4:master Mar 7, 2024
4 checks passed
@Enet4 Enet4 added this to the DICOM-rs 0.7 milestone Mar 13, 2024
@jmlaka jmlaka deleted the datetimefix branch November 10, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lib Area: library breaking change Hint that this may require a major version bump on release bug This is a bug C-core Crate: dicom-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DICOM naive datetime retrieval
2 participants