Enet4 / dicom-rs

Rust implementation of the DICOM standard
https://dicom-rs.github.io
Apache License 2.0
404 stars 76 forks source link

Leading whitespaces in VR DS and IS #335

Closed lalberge closed 1 year ago

lalberge commented 1 year ago

Bug: Leading whitespaces in DS or IS would cause errors in conversion from a string primitive. The issue is described here: https://github.com/Enet4/dicom-rs/issues/333

Cause: Strings were not trimmed with leading spaces.

Fix: Added .trim_start() before conversion to int or float.

Verification: Added unit tests to ensure theses cases are now adequately handled.

Enet4 commented 1 year ago

Looks good! We are only missing some updates to the documentation, since it still indicates that it only trims trailing whitespace. Please see lines 926, 1085, 1274, 1434, 1783 and update them accordingly. It also seems that one of the methods was missing a similar sentence, which would have been in line 1623. We can fix that too while we're at it.

I would go with something like this:

--- a/core/src/value/primitive.rs
+++ b/core/src/value/primitive.rs
@@ -923,8 +923,7 @@ impl PrimitiveValue {
     /// If the value is a string or sequence of strings,
     /// the first string is parsed to obtain an integer,
     /// potentially failing if the string does not represent a valid integer.
-    /// The string is stripped of trailing whitespace before parsing,
-    /// in order to account for the possible padding to even length.
+    /// The string is stripped of leading/trailing whitespace before parsing.
     /// If the value is a sequence of U8 bytes,
     /// the bytes are individually interpreted as independent numbers.
lalberge commented 1 year ago

Looks good! We are only missing some updates to the documentation, since it still indicates that it only trims trailing whitespace. Please see lines 926, 1085, 1274, 1434, 1783 and update them accordingly. It also seems that one of the methods was missing a similar sentence, which would have been in line 1623. We can fix that too while we're at it.

I would go with something like this:

--- a/core/src/value/primitive.rs
+++ b/core/src/value/primitive.rs
@@ -923,8 +923,7 @@ impl PrimitiveValue {
     /// If the value is a string or sequence of strings,
     /// the first string is parsed to obtain an integer,
     /// potentially failing if the string does not represent a valid integer.
-    /// The string is stripped of trailing whitespace before parsing,
-    /// in order to account for the possible padding to even length.
+    /// The string is stripped of leading/trailing whitespace before parsing.
     /// If the value is a sequence of U8 bytes,
     /// the bytes are individually interpreted as independent numbers.

Done 👍

lalberge commented 1 year ago

Let me know if there is anything more to do :) Thanks for the support