Revamp DICOM datetime by jmlaka · Pull Request #453 · Enet4/dicom-rs

jmlaka · 2024-02-08T20:01:27Z

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 !

- deprecation messages - add basic D/T values to prelude - clippy fixes

- default handling of ambiguous DT Ranges - needs chrono 'clock' feature

- doc changes

jmlaka · 2024-02-15T13:38:46Z

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

Enet4

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

core/src/value/partial.rs

object/src/mem.rs

core/src/value/primitive.rs

- fix doctest

jmlaka · 2024-02-21T19:22:58Z

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

Enet4

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

core/src/value/range.rs

core/src/value/partial.rs

object/src/mem.rs

- fix doc links

core/src/value/mod.rs

- 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 · 2024-03-02T16:24:36Z

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

jmlaka · 2024-03-02T20:37:51Z

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

Enet4

Thank you once again. 👍 We're bringing this in.

jmlaka added 7 commits January 28, 2024 14:25

- 1st commit

635b3be

- doc tests need fixing

7df0599

- some doctests work

fa063a7

- change precision api and it's docs

8f57b3f

- added ambiguous dt range variants

5d14d04

- remove Precision trait ?

c49f653

- fix doc test

1b9ae35

- deprecation messages - add basic D/T values to prelude - clippy fixes

jmlaka marked this pull request as draft February 10, 2024 16:25

jmlaka added 3 commits February 10, 2024 23:05

- simplify DateTimeRange

8ee3357

- default handling of ambiguous DT Ranges - needs chrono 'clock' feature

- add possibility to fail on ambiguous DT range

cc30c98

- primitive value of da, tm, dt can be retrieved as it's range

73c3582

jmlaka marked this pull request as ready for review February 13, 2024 14:30

- as default, ambiguous DT range uses local clock time-zone

219814d

- doc changes

Enet4 reviewed Feb 17, 2024

View reviewed changes

jmlaka added 2 commits February 21, 2024 19:22

- fix deprecation info

e784fda

- fix doctest

- simplify DicomDateTime api

1a8621b

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 self-requested a review February 23, 2024 11:23

some typos

5e74ca9

Enet4 reviewed Feb 24, 2024

View reviewed changes

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

- rename PreciseDateTimeResult

7487735

- fix doc links

Enet4 reviewed Mar 1, 2024

View reviewed changes

core/src/value/mod.rs Outdated Show resolved Hide resolved

Enet4 added 3 commits March 1, 2024 18:00

Update core/src/value/mod.rs

a4970c4

[core] Improve PreciseDateTime

3253ce2

- clarify its purpose in the documentation - document variants - rename existing methods and add more methods

[core] custom impl PartialOrd for PreciseDateTime

aeea024

- do not provide an order between non-matching variants

Enet4 added 5 commits March 2, 2024 15:38

[core] Fix doctest in PrimitiveValue

b62e079

[core] move PreciseDateTime to partial module

a77c0cf

[core] Improve docs of parse_datetime and parse_datetime_partial

b6b794d

[core] impl FromStr for DicomDateTime

406bd28

[core] Improve docs of AmbiguousDtRangeParser and impls

0d9df8a

Enet4 changed the title ~~Datetimefix~~ Revamp DICOM datetime Mar 2, 2024

Enet4 approved these changes Mar 7, 2024

View reviewed changes

Enet4 merged commit 070ebe9 into Enet4:master Mar 7, 2024

Enet4 added this to the DICOM-rs 0.7 milestone Mar 13, 2024

jmlaka deleted the datetimefix branch November 10, 2024 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp DICOM datetime#453

Revamp DICOM datetime#453
Enet4 merged 23 commits intoEnet4:masterfrom
jmlaka:datetimefix

jmlaka commented Feb 8, 2024

Uh oh!

jmlaka commented Feb 15, 2024

Uh oh!

Enet4 left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmlaka commented Feb 21, 2024

Uh oh!

Enet4 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Enet4 commented Mar 2, 2024

Uh oh!

jmlaka commented Mar 2, 2024

Uh oh!

Enet4 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jmlaka commented Feb 8, 2024

Uh oh!

jmlaka commented Feb 15, 2024

Uh oh!

Enet4 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmlaka commented Feb 21, 2024

Uh oh!

Enet4 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Enet4 commented Mar 2, 2024

Uh oh!

jmlaka commented Mar 2, 2024

Uh oh!

Enet4 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Enet4 left a comment •

edited

Loading