Enet4 / dicom-rs

Rust implementation of the DICOM standard
https://dicom-rs.github.io
Apache License 2.0
403 stars 75 forks source link

Revamp DICOM datetime #453

Closed jmlaka closed 4 months ago

jmlaka commented 4 months ago

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 commented 4 months ago

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

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

The corecrate 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

jmlaka commented 4 months ago

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

Enet4 commented 4 months ago

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

jmlaka commented 4 months ago

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