Specimen description and preparation step fixes

ImagingDataCommons / highdicom

High-level DICOM abstractions for the Python programming language

https://highdicom.readthedocs.io

MIT License

164 stars 34 forks source link

Specimen description and preparation step fixes #246

Closed erikogabrielsson closed 8 months ago

erikogabrielsson commented 11 months ago

Hi,

I had some problems with using the 'SpecimenDescriptionandSpecimenPreparationStep` content items:

The name for the DateTimeContentItem was incorrectly capitalized.
Datetimes would not parse correctly.
Some attributes in the standard were missing.

This PR fixes the above issues and adds tests for the bugs and added parameters. I'm not sureif the approach and style is in line with the rest of the code or if these changes breaks anything else. I'm happy to receive feedback if that is the case.

CPBridge commented 11 months ago

Thanks @erikogabrielsson I'll take look!

CPBridge commented 10 months ago

Oh and one more request, sorry. Since this involves API changes could you please change the target branch of the pull request from master to v0.22.0dev and we'llaim to release the changes in the next minor release. Thanks

erikogabrielsson commented 10 months ago

Hi @CPBridge and thanks for the review. No worries regarding the timing.

I think I have addressed all of your comments with new commits. Please re-review and let me know if anything was missed.

CPBridge commented 10 months ago

Thanks @erikogabrielsson, your changes look good. However it seems that some comments from my review may have been missed, probably because of github's annoying habit of "collapsing" comments in the middle of a review?

Specifically I'm talking about this, this and this.

erikogabrielsson commented 10 months ago

Yes I had missed those (they were collapsed). I have now addressed the missed comments.

CPBridge commented 10 months ago

PS I'll give @hackermd a few more days to comment if he likes before merging