Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[json] Allow null for string values #464

Merged
merged 1 commit into from
Mar 10, 2024

Conversation

feliwir
Copy link
Contributor

@feliwir feliwir commented Mar 7, 2024

@Enet4 Enet4 added bug This is a bug A-lib Area: library C-json Crate: dicom-json labels Mar 7, 2024
@feliwir feliwir changed the title Allow null for string values [json] Allow null for string values Mar 7, 2024
@feliwir feliwir force-pushed the allow-null-for-string-values branch from e941328 to 200c819 Compare March 7, 2024 14:26
@feliwir feliwir force-pushed the allow-null-for-string-values branch from 200c819 to 7859de5 Compare March 8, 2024 08:18
Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this indeed works! I will approve and merge while leaving these notes on potential future changes:

  • There is an extra allocation from Vec<Option<Value>> to Vec<Value>, but this also happens in a few other places as well. Maybe someone could find some time to fine-tune these later.
  • This takes care of the conversion from DICOM JSON to in-memory representations, but, thinking reciprocally, I suspect that empty values will still be encoded to DICOM JSON as empty strings with the current implementation. This is worth taking care of in a new pull request.

Thanks again for your contribution @feliwir. 👍

@Enet4 Enet4 merged commit 4216b0a into Enet4:master Mar 10, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lib Area: library bug This is a bug C-json Crate: dicom-json
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants