Add into_text_value() to PrimitiveValues#718
Conversation
d0e4053 to
f3af414
Compare
Enet4
left a comment
There was a problem hiding this comment.
Thank you for your pull request. Please let me know if you are available to reiterate on it per the comments below.
The only major concern that makes me put this on hold is the fact that it can easily create invalid date-time values.
core/src/value/primitive.rs
Outdated
| /// # When to use `into_text_value()` vs `to_raw_str()` | ||
| /// | ||
| /// Use **`to_raw_str()`** when you need a string representation | ||
| /// for display, comparison, or serialization: | ||
| /// - Returns `Cow<'_, str>` (borrows when possible) | ||
| /// - Does not consume the original value | ||
| /// - Suitable for temporary string operations | ||
| /// | ||
| /// Use **`into_text_value()`** when you need to store or manipulate | ||
| /// the value as text within the `PrimitiveValue` enum: | ||
| /// - Returns `PrimitiveValue::Str` (consumes the original) | ||
| /// - Useful for normalizing values to text representation | ||
| /// - Allows further processing as a `PrimitiveValue` | ||
| /// | ||
| /// [`to_raw_str()`]: #method.to_raw_str |
There was a problem hiding this comment.
It feels much too verbose to discuss when to use into_text_value() or some other method, frankly to the point of not being useful. The primary differences are also described in the type signature, through the receiving type (self vs &self) and the output type (PrimitiveValue vs one of Rust string types). Also, users are still much more likely to use to_str over to_raw_str or into_text_value.
I'd frame it like this:
| /// # When to use `into_text_value()` vs `to_raw_str()` | |
| /// | |
| /// Use **`to_raw_str()`** when you need a string representation | |
| /// for display, comparison, or serialization: | |
| /// - Returns `Cow<'_, str>` (borrows when possible) | |
| /// - Does not consume the original value | |
| /// - Suitable for temporary string operations | |
| /// | |
| /// Use **`into_text_value()`** when you need to store or manipulate | |
| /// the value as text within the `PrimitiveValue` enum: | |
| /// - Returns `PrimitiveValue::Str` (consumes the original) | |
| /// - Useful for normalizing values to text representation | |
| /// - Allows further processing as a `PrimitiveValue` | |
| /// | |
| /// [`to_raw_str()`]: #method.to_raw_str | |
| /// # When to use `into_text_value()` | |
| /// | |
| /// You can use `into_text_value` to | |
| /// turn an existing DICOM value into a textual DICOM value | |
| /// stored in a single string underneath, | |
| /// regardless of the value's original format. | |
| /// It is also an alternative to converting each number into strings | |
| /// before they are encased in `PrimitiveValue`. | |
| /// | |
| /// The methods [`to_str`] or [`to_multi_str`] | |
| /// would be preferred when the intent is to | |
| /// retrieve the underlying values as a string type. | |
| /// | |
| /// [`to_str`]: PrimitiveValue::to_str | |
| /// [`to_multi_str`]: PrimitiveValue::to_multi_str | |
| /// [`to_raw_str()`]: PrimitiveValue::to_raw_str |
There was a problem hiding this comment.
I agree it's too verbose. It was a combination of me trying to emulate the other documentation in this file, which is fairly verbose and trying to actually understand the implications myself. I especially like the last point to call the string methods if the desired result is a string.
Accepted
There was a problem hiding this comment.
The description is still very verbose after these changes. Are we ok with the state of the documentation after this change or do you want me to make the examples more concise. Some of them are probably not required - we really just need two or so examples to convey that it's just .to_raw_str() underneath the covers.
There was a problem hiding this comment.
It will be a bit more than to_raw_str in the end (because of the date-time edge-case), but in any case I think its extensiveness is not a big issue at this point. Eventually we could get rid of the "When to use into_text_value" heading (keeping the contents below it), which will automatically give it a feeling of conciseness.
| /// | ||
| /// assert_eq!( | ||
| /// text_value, | ||
| /// PrimitiveValue::Str("2024-12-25".to_string()) |
There was a problem hiding this comment.
This shows... an interesting caveat. Dates are only DICOM-encoded correctly when converted via to_encoded, but to_raw_str does not use it (which would be fine because to_str and to_raw_str are ill-advised for DICOM serialization anyway, so they are not expected to be pushed back into a DICOM value).
The current behavior of this method will come across as surprising, so we will have to adjust it accordingly.
- Date and date-time values in
PrimitiveValue::DateandPrimitiveValue::DateTimeare encoded to their standard DICOM textual form.- Other binary variants are converted to strings via
to_string.
There was a problem hiding this comment.
I can work on this. For clarification, is it as simple as calling to_encoded() when appropriate and updating the docs? Or am I missing some nuance?
There was a problem hiding this comment.
So yes, for the variants Da and Dt they should be converted to text via to_encoded(). The rule for concatenating multiple values into a single string would be the same (joined together with backslash as the separator).
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
Implements: ISSUE-88
This adds
into_text_value()forPrimitiveValueas well as animpl From<Cow<'_, str>> for PrimitiveValueper the suggestion in the issue above.Assumptions to check:
PrimitiveValue::Str(includingEmptyandStrs)to_raw_str()instead ofto_str()to preserve the most information (no trimming of extra spaces, etc)into_text_value()andimpl From<Cow<'_, str>> for PrimitiveValueare useful for convenience/completeness, even thoughinto_text_value()is justPrimitiveValue::from(self.to_raw_str())Other things to check:
to_raw_str()tests. Let me know if I should.