Specimen description and preparation step fixes #246
Conversation
|
Thanks @erikogabrielsson I'll take look! |
There was a problem hiding this comment.
Hi @erikogabrielsson , sorry it took so long to look at this properly. Your help with this is very much appreciated!
Overall looks great but I have some comments on the specifics
Thanks!
| raise ValueError( | ||
| f'Could not decode time value "{self.Time}"' | ||
| ) from exception | ||
| return value.replace() |
There was a problem hiding this comment.
I didn't write this code originally (that was @hackermd ) and I'm not sure why it was written like it is nor why it doesn't work. Your approach seems more logical I have to say. But have you tried it with pydicom.config.datetime_conversion?
There was a problem hiding this comment.
I added config.datetime_conversion = datetime_conversion as parameter to the test_from_dataset and it looks OK. Looking at the DA, TM, and DT inits they either take a string or a date, time, or datetime.
|
Oh and one more request, sorry. Since this involves API changes could you please change the target branch of the pull request from |
Co-authored-by: Chris Bridge <chrisbridge44@googlemail.com>
…ikogabrielsson/highdicom into SpecimenPreparationStep-fixes
…nfig.datetime_conversion setting
|
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. |
|
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? |
Co-authored-by: Chris Bridge <chrisbridge44@googlemail.com>
Co-authored-by: Chris Bridge <chrisbridge44@googlemail.com>
|
Yes I had missed those (they were collapsed). I have now addressed the missed comments. |
There was a problem hiding this comment.
Thanks for your hard work on this @erikogabrielsson, we'll put these changes 0.22.0 (hopefullly not too long until I get around to this)
|
PS I'll give @hackermd a few more days to comment if he likes before merging |
Hi,
I had some problems with using the 'SpecimenDescription
andSpecimenPreparationStep` content items: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.