search

dcmjs-org / dcmjs

Javascript implementation of DICOM manipulation
https://dcmjs.netlify.com/
MIT License
296 stars 112 forks source link

Convert empty DecimalString and NumberString to null instead of zero #278

Closed richard-viney closed 2 years ago

richard-viney commented 2 years ago

This is needed when doing C-FIND with dcmjs-dimse, where "" means 'match everything', not zero.

pieper commented 2 years ago

Makes sense to me. Can you confirm that it results in an empty field when writing out again? Looks to me like it will.

richard-viney commented 2 years ago

Nice catch, I've pushed a fix for it writing back "null" in the serialisation case. It now writes back the empty string again so the read=>write roundtrip with a "" value in IntegerString/DecimalString now keeps it unchanged, as one would expect (this wasn't the case before this PR, i.e it would be transformed to zero).

richard-viney commented 2 years ago

Also worth noting that this is a breaking API change and so would need a v0.23 release.

pieper commented 2 years ago

Also worth noting that this is a breaking API change and so would need a v0.23 release.

Note that we use these commit message conventions so this would trigger a new major version, which seems a bit much for this. Maybe this is better seen as a fix, since as you pointed out the behavior was basically incorrect before.

richard-viney commented 2 years ago

Also worth noting that this is a breaking API change and so would need a v0.23 release.

Note that we use these commit message conventions so this would trigger a new major version, which seems a bit much for this. Maybe this is better seen as a fix, since as you pointed out the behavior was basically incorrect before.

Sounds good to me.

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 0.23.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: