Always display NA as unquoted in the Variables pane (#432)

* Always display NA as unquoted in the Variables pane * Rename a struct and a function * Add a test * Move FormatOptions to vector module * Inline format_with_options() * Tweak test to check handling of a string containing " * Add a note about quote option
posit-dev · Jul 15, 2024 · 7e16d1a · 7e16d1a
1 parent ad73e15
commit 7e16d1a
Show file tree

Hide file tree

Showing 9 changed files with 56 additions and 51 deletions.
diff --git a/crates/harp/src/vector/character_vector.rs b/crates/harp/src/vector/character_vector.rs
@@ -18,6 +18,7 @@ use libr::STRSXP;
 
 use crate::object::RObject;
 use crate::utils::r_str_to_owned_utf8_unchecked;
+use crate::vector::FormatOptions;
 use crate::vector::Vector;
 
 #[harp_macros::vector]
@@ -80,8 +81,16 @@ impl Vector for CharacterVector {
         r_str_to_owned_utf8_unchecked(*x)
     }
 
-    fn format_one(&self, x: Self::Type) -> String {
-        x
+    fn format_one(&self, x: Self::Type, options: Option<&FormatOptions>) -> String {
+        if let Some(&FormatOptions { quote, .. }) = options {
+            if quote {
+                format!(r#""{}""#, x.replace('"', r#"\""#))
+            } else {
+                x
+            }
+        } else {
+            x
+        }
     }
 }
 

diff --git a/crates/harp/src/vector/complex_vector.rs b/crates/harp/src/vector/complex_vector.rs
@@ -15,6 +15,7 @@ use libr::DATAPTR;
 use libr::SEXP;
 
 use crate::object::RObject;
+use crate::vector::FormatOptions;
 use crate::vector::Vector;
 
 #[derive(Debug, PartialEq, Clone, Copy)]
@@ -81,7 +82,7 @@ impl Vector for ComplexVector {
         *x
     }
 
-    fn format_one(&self, x: Self::Type) -> String {
+    fn format_one(&self, x: Self::Type, _option: Option<&FormatOptions>) -> String {
         format!("{}+{}i", x.r.to_string(), x.i.to_string())
     }
 }
diff --git a/crates/harp/src/vector/factor.rs b/crates/harp/src/vector/factor.rs
@@ -16,6 +16,7 @@ use libr::SEXP;
 
 use crate::object::RObject;
 use crate::r_symbol;
+use crate::vector::FormatOptions;
 use crate::vector::CharacterVector;
 use crate::vector::Vector;
 
@@ -73,7 +74,7 @@ impl Vector for Factor {
         *x
     }
 
-    fn format_one(&self, x: Self::Type) -> String {
+    fn format_one(&self, x: Self::Type, _option: Option<&FormatOptions>) -> String {
         self.levels.get_unchecked((x - 1) as isize).unwrap()
     }
 }
diff --git a/crates/harp/src/vector/formatted_vector.rs b/crates/harp/src/vector/formatted_vector.rs
@@ -26,6 +26,7 @@ use crate::utils::r_typeof;
 use crate::vector::CharacterVector;
 use crate::vector::ComplexVector;
 use crate::vector::Factor;
+use crate::vector::FormatOptions;
 use crate::vector::IntegerVector;
 use crate::vector::LogicalVector;
 use crate::vector::NumericVector;
@@ -47,7 +48,7 @@ pub enum FormattedVector {
     },
     Character {
         vector: CharacterVector,
-        options: FormattedVectorCharacterOptions,
+        options: FormatOptions,
     },
     Complex {
         vector: ComplexVector,
@@ -58,25 +59,18 @@ pub enum FormattedVector {
     },
     FormattedVector {
         vector: CharacterVector,
-        options: FormattedVectorCharacterOptions,
+        options: FormatOptions,
     },
 }
 
-// Formatting options for character vectors
-pub struct FormattedVectorCharacterOptions {
-    // Wether to quote the strings or not (defaults to `true`)
-    // If `true`, elements will be quoted during format so, eg: c("a", "b") becomes ("\"a\"", "\"b\"") in Rust
-    pub quote: bool,
-}
-
 // Formatting options for vectors
 #[derive(Default)]
 pub struct FormattedVectorOptions {
     // Formatting options for character vectors
-    pub character: FormattedVectorCharacterOptions,
+    pub character: FormatOptions,
 }
 
-impl Default for FormattedVectorCharacterOptions {
+impl Default for FormatOptions {
     fn default() -> Self {
         Self { quote: true }
     }
@@ -139,35 +133,21 @@ impl FormattedVector {
 
     pub fn get_unchecked(&self, index: isize) -> String {
         match self {
-            FormattedVector::Raw { vector } => vector.format_elt_unchecked(index),
-            FormattedVector::Logical { vector } => vector.format_elt_unchecked(index),
-            FormattedVector::Integer { vector } => vector.format_elt_unchecked(index),
-            FormattedVector::Numeric { vector } => vector.format_elt_unchecked(index),
+            FormattedVector::Raw { vector } => vector.format_elt_unchecked(index, None),
+            FormattedVector::Logical { vector } => vector.format_elt_unchecked(index, None),
+            FormattedVector::Integer { vector } => vector.format_elt_unchecked(index, None),
+            FormattedVector::Numeric { vector } => vector.format_elt_unchecked(index, None),
             FormattedVector::Character { vector, options } => {
-                let value = vector.format_elt_unchecked(index);
-                self.format_with_string_options(value, options)
+                vector.format_elt_unchecked(index, Some(options))
             },
-            FormattedVector::Complex { vector } => vector.format_elt_unchecked(index),
-            FormattedVector::Factor { vector } => vector.format_elt_unchecked(index),
+            FormattedVector::Complex { vector } => vector.format_elt_unchecked(index, None),
+            FormattedVector::Factor { vector } => vector.format_elt_unchecked(index, None),
             FormattedVector::FormattedVector { vector, options } => {
-                let value = vector.format_elt_unchecked(index);
-                self.format_with_string_options(value, options)
+                vector.format_elt_unchecked(index, Some(options))
             },
         }
     }
 
-    fn format_with_string_options(
-        &self,
-        value: String,
-        options: &FormattedVectorCharacterOptions,
-    ) -> String {
-        if options.quote {
-            format!("\"{}\"", value.replace("\"", "\\\""))
-        } else {
-            value
-        }
-    }
-
     pub fn len(&self) -> isize {
         unsafe { Rf_xlength(self.data()) }
     }
@@ -244,14 +224,14 @@ mod tests {
     use libr::STRSXP;
 
     use crate::environment::Environment;
+    use crate::environment::R_ENVS;
     use crate::eval::r_parse_eval0;
     use crate::modules::HARP_ENV;
-    use crate::object::RObject;
     use crate::r_assert_type;
     use crate::test::r_test;
     use crate::vector::formatted_vector::FormattedVector;
-    use crate::vector::formatted_vector::FormattedVectorCharacterOptions;
     use crate::vector::formatted_vector::FormattedVectorOptions;
+    use crate::vector::FormatOptions;
 
     #[test]
     fn test_unconforming_format_method() {
@@ -283,24 +263,26 @@ mod tests {
     #[test]
     fn test_formatting_option() {
         r_test(|| {
-            let x = RObject::from(vec![String::from("1"), String::from("2")]);
+            let x =
+                r_parse_eval0(r#"c("1", "2", '"a"', "NA", NA_character_)"#, R_ENVS.base).unwrap();
             r_assert_type(x.sexp, &[STRSXP]).unwrap();
 
             let formatted = FormattedVector::new_with_options(x.sexp, FormattedVectorOptions {
-                character: FormattedVectorCharacterOptions { quote: false },
+                character: FormatOptions { quote: false },
             })
             .unwrap();
 
             let out = formatted.iter().join(" ");
-            assert_eq!(out, String::from("1 2"));
+            assert_eq!(out, String::from(r#"1 2 "a" NA NA"#));
 
             let formatted = FormattedVector::new_with_options(x.sexp, FormattedVectorOptions {
-                character: FormattedVectorCharacterOptions { quote: true },
+                character: FormatOptions { quote: true },
             })
             .unwrap();
 
+            // NA is always unquoted regardless of the quote option
             let out = formatted.iter().join(" ");
-            assert_eq!(out, String::from("\"1\" \"2\""));
+            assert_eq!(out, String::from(r#""1" "2" "\"a\"" "NA" NA"#));
         })
     }
 }
diff --git a/crates/harp/src/vector/integer_vector.rs b/crates/harp/src/vector/integer_vector.rs
@@ -14,6 +14,7 @@ use libr::INTSXP;
 use libr::SEXP;
 
 use crate::object::RObject;
+use crate::vector::FormatOptions;
 use crate::vector::Vector;
 
 #[harp_macros::vector]
@@ -68,7 +69,7 @@ impl Vector for IntegerVector {
         *x
     }
 
-    fn format_one(&self, x: Self::Type) -> String {
+    fn format_one(&self, x: Self::Type, _option: Option<&FormatOptions>) -> String {
         x.to_string()
     }
 }
diff --git a/crates/harp/src/vector/logical_vector.rs b/crates/harp/src/vector/logical_vector.rs
@@ -14,6 +14,7 @@ use libr::LOGICAL_ELT;
 use libr::SEXP;
 
 use crate::object::RObject;
+use crate::vector::FormatOptions;
 use crate::vector::Vector;
 
 #[harp_macros::vector]
@@ -68,7 +69,7 @@ impl Vector for LogicalVector {
         *x == 1
     }
 
-    fn format_one(&self, x: Self::Type) -> String {
+    fn format_one(&self, x: Self::Type, _option: Option<&FormatOptions>) -> String {
         if x {
             String::from("TRUE")
         } else {

diff --git a/crates/harp/src/vector/mod.rs b/crates/harp/src/vector/mod.rs
@@ -37,6 +37,14 @@ pub use raw_vector::RawVector;
 pub mod formatted_vector;
 pub mod names;
 
+// Formatting options for character vectors
+pub struct FormatOptions {
+    // Wether to quote the strings or not (defaults to `true`)
+    // If `true`, elements will be quoted during format so, eg: c("a", "b") becomes ("\"a\"", "\"b\"") in Rust
+    // Currently, this option is meaningful only for a character vector and is ignored on other types
+    pub quote: bool,
+}
+
 pub trait Vector {
     type Type;
     type Item: ?Sized;
@@ -100,11 +108,11 @@ pub trait Vector {
         Rf_xlength(self.data()) as usize
     }
 
-    fn format_one(&self, x: Self::Type) -> String;
+    fn format_one(&self, x: Self::Type, options: Option<&FormatOptions>) -> String;
 
-    fn format_elt_unchecked(&self, index: isize) -> String {
+    fn format_elt_unchecked(&self, index: isize, options: Option<&FormatOptions>) -> String {
         match self.get_unchecked(index) {
-            Some(x) => self.format_one(x),
+            Some(x) => self.format_one(x, options),
             None => String::from("NA"),
         }
     }

diff --git a/crates/harp/src/vector/numeric_vector.rs b/crates/harp/src/vector/numeric_vector.rs
@@ -14,6 +14,7 @@ use libr::REAL_ELT;
 use libr::SEXP;
 
 use crate::object::RObject;
+use crate::vector::FormatOptions;
 use crate::vector::Vector;
 
 #[harp_macros::vector]
@@ -68,7 +69,7 @@ impl Vector for NumericVector {
         *x
     }
 
-    fn format_one(&self, x: Self::Type) -> String {
+    fn format_one(&self, x: Self::Type, _option: Option<&FormatOptions>) -> String {
         x.to_string()
     }
 }
diff --git a/crates/harp/src/vector/raw_vector.rs b/crates/harp/src/vector/raw_vector.rs
@@ -13,6 +13,7 @@ use libr::RAW_ELT;
 use libr::SEXP;
 
 use crate::object::RObject;
+use crate::vector::FormatOptions;
 use crate::vector::Vector;
 
 #[harp_macros::vector]
@@ -67,7 +68,7 @@ impl Vector for RawVector {
         *x
     }
 
-    fn format_one(&self, x: Self::Type) -> String {
+    fn format_one(&self, x: Self::Type, _option: Option<&FormatOptions>) -> String {
         format!("{:02x}", x)
     }
 }