Do some str collect cleanup (#312)

crates/nu-command/src/conversions/into/string.rs

@@ -212,7 +212,7 @@ pub fn action(
         },
 
         Value::Filesize { val: _, .. } => Value::String {
-            val: input.clone().into_string(),
+            val: input.clone().into_string(", "),
             span,
         },
         Value::Nothing { .. } => Value::String {

crates/nu-command/src/strings/build_string.rs

@@ -52,7 +52,7 @@ impl Command for BuildString {
         let output = call
             .positional
             .iter()
-            .map(|expr| eval_expression(engine_state, stack, expr).map(|val| val.into_string()))
+            .map(|expr| eval_expression(engine_state, stack, expr).map(|val| val.into_string(", ")))
             .collect::<Result<Vec<String>, ShellError>>()?;
 
         Ok(Value::String {

crates/nu-command/src/strings/str_/collect.rs

@@ -34,31 +34,25 @@ impl Command for StrCollect {
     ) -> Result<PipelineData, ShellError> {
         let separator: Option<String> = call.opt(engine_state, stack, 0)?;
 
+        // Hmm, not sure what we actually want. If you don't use debug_string, Date comes out as human readable
+        // which feels funny
         #[allow(clippy::needless_collect)]
-        let strings: Vec<Result<String, ShellError>> =
+        let strings: Vec<String> = input
-            input.into_iter().map(|value| value.as_string()).collect();
+            .into_iter()
-        let strings: Result<Vec<_>, _> = strings.into_iter().collect::<Result<_, _>>();
+            .map(|value| value.debug_string("\n"))
+            .collect();
 
-        match strings {
+        let output = if let Some(separator) = separator {
-            Ok(strings) => {
+            strings.join(&separator)
-                let output = if let Some(separator) = separator {
+        } else {
-                    strings.join(&separator)
+            strings.join("")
-                } else {
+        };
-                    strings.join("")
-                };
 
-                Ok(Value::String {
+        Ok(Value::String {
-                    val: output,
+            val: output,
-                    span: call.head,
+            span: call.head,
-                }
-                .into_pipeline_data())
-            }
-            _ => Err(ShellError::CantConvert(
-                "string".into(),
-                "non-string input".into(),
-                call.head,
-            )),
         }
+        .into_pipeline_data())
     }
 
     fn examples(&self) -> Vec<Example> {

crates/nu-command/src/system/run_external.rs

@@ -111,7 +111,7 @@ impl ExternalCommand {
                                     }
                                 }
                                 x => {
-                                    if stdin_write.write(x.into_string().as_bytes()).is_err() {
+                                    if stdin_write.write(x.into_string(", ").as_bytes()).is_err() {
                                         return Err(());
                                     }
                                 }

crates/nu-command/src/viewers/griddle.rs

@@ -94,7 +94,7 @@ prints out the list properly."#
                 let mut items = vec![];
 
                 for (i, (c, v)) in cols.into_iter().zip(vals.into_iter()).enumerate() {
-                    items.push((i, c, v.into_string()))
+                    items.push((i, c, v.into_string(", ")))
                 }
 
                 Ok(create_grid_output2(
@@ -187,7 +187,7 @@ fn convert_to_list2(iter: impl IntoIterator<Item = Value>) -> Option<Vec<(usize,
             let mut row = vec![row_num.to_string()];
 
             if headers.is_empty() {
-                row.push(item.into_string())
+                row.push(item.into_string(", "))
             } else {
                 for header in headers.iter().skip(1) {
                     let result = match item {
@@ -201,7 +201,7 @@ fn convert_to_list2(iter: impl IntoIterator<Item = Value>) -> Option<Vec<(usize,
                     };
 
                     match result {
-                        Ok(value) => row.push(value.into_string()),
+                        Ok(value) => row.push(value.into_string(", ")),
                         Err(_) => row.push(String::new()),
                     }
                 }

crates/nu-command/src/viewers/table.rs

@@ -76,7 +76,7 @@ impl Command for Table {
                             style: nu_table::TextStyle::default_field(),
                         },
                         StyledString {
-                            contents: v.into_string(),
+                            contents: v.into_string(", "),
                             style: nu_table::TextStyle::default(),
                         },
                     ])
@@ -123,7 +123,7 @@ fn convert_to_table(
             let mut row = vec![row_num.to_string()];
 
             if headers.is_empty() {
-                row.push(item.into_string())
+                row.push(item.into_string(", "))
             } else {
                 for header in headers.iter().skip(1) {
                     let result = match item {
@@ -137,7 +137,7 @@ fn convert_to_table(
                     };
 
                     match result {
-                        Ok(value) => row.push(value.into_string()),
+                        Ok(value) => row.push(value.into_string(", ")),
                         Err(_) => row.push(String::new()),
                     }
                 }

crates/nu-engine/src/from_value.rs

@@ -95,14 +95,14 @@ impl FromValue for f64 {
 impl FromValue for String {
     fn from_value(v: &Value) -> Result<Self, ShellError> {
         // FIXME: we may want to fail a little nicer here
-        Ok(v.clone().into_string())
+        Ok(v.clone().into_string(", "))
     }
 }
 
 impl FromValue for Spanned<String> {
     fn from_value(v: &Value) -> Result<Self, ShellError> {
         Ok(Spanned {
-            item: v.clone().into_string(),
+            item: v.clone().into_string(", "),
             span: v.span()?,
         })
     }

crates/nu-protocol/src/pipeline_data.rs

@@ -53,8 +53,8 @@ impl PipelineData {
 
     pub fn collect_string(self) -> String {
         match self {
-            PipelineData::Value(v) => v.collect_string(),
+            PipelineData::Value(v) => v.into_string("\n"),
-            PipelineData::Stream(s) => s.collect_string(),
+            PipelineData::Stream(s) => s.into_string("\n"),
         }
     }
 

crates/nu-protocol/src/value/mod.rs

@@ -87,14 +87,11 @@ impl Value {
     pub fn as_string(&self) -> Result<String, ShellError> {
         match self {
             Value::String { val, .. } => Ok(val.to_string()),
-            x => {
+            x => Err(ShellError::CantConvert(
-                println!("{:?}", x);
+                "string".into(),
-                Err(ShellError::CantConvert(
+                x.get_type().to_string(),
-                    "string".into(),
+                self.span()?,
-                    x.get_type().to_string(),
+            )),
-                    self.span()?,
-                ))
-            }
         }
     }
 
@@ -166,7 +163,7 @@ impl Value {
     }
 
     /// Convert Value into string. Note that Streams will be consumed.
-    pub fn into_string(self) -> String {
+    pub fn into_string(self, separator: &str) -> String {
         match self {
             Value::Bool { val, .. } => val.to_string(),
             Value::Int { val, .. } => val.to_string(),
@@ -175,23 +172,27 @@ impl Value {
             Value::Duration { val, .. } => format_duration(val),
             Value::Date { val, .. } => HumanTime::from(val).to_string(),
             Value::Range { val, .. } => {
-                format!("{}..{}", val.from.into_string(), val.to.into_string())
+                format!(
+                    "{}..{}",
+                    val.from.into_string(", "),
+                    val.to.into_string(", ")
+                )
             }
             Value::String { val, .. } => val,
             Value::List { vals: val, .. } => format!(
                 "[{}]",
                 val.into_iter()
-                    .map(|x| x.into_string())
+                    .map(|x| x.into_string(", "))
                     .collect::<Vec<_>>()
-                    .join(", ")
+                    .join(separator)
             ),
             Value::Record { cols, vals, .. } => format!(
                 "{{{}}}",
                 cols.iter()
                     .zip(vals.iter())
-                    .map(|(x, y)| format!("{}: {}", x, y.clone().into_string()))
+                    .map(|(x, y)| format!("{}: {}", x, y.clone().into_string(", ")))
                     .collect::<Vec<_>>()
-                    .join(", ")
+                    .join(separator)
             ),
             Value::Block { val, .. } => format!("<Block {}>", val),
             Value::Nothing { .. } => String::new(),
@@ -201,7 +202,8 @@ impl Value {
         }
     }
 
-    pub fn collect_string(self) -> String {
+    /// Convert Value into string. Note that Streams will be consumed.
+    pub fn debug_string(self, separator: &str) -> String {
         match self {
             Value::Bool { val, .. } => val.to_string(),
             Value::Int { val, .. } => val.to_string(),
@@ -210,24 +212,33 @@ impl Value {
             Value::Duration { val, .. } => format_duration(val),
             Value::Date { val, .. } => format!("{:?}", val),
             Value::Range { val, .. } => {
-                format!("{}..{}", val.from.into_string(), val.to.into_string())
+                format!(
+                    "{}..{}",
+                    val.from.into_string(", "),
+                    val.to.into_string(", ")
+                )
             }
             Value::String { val, .. } => val,
-            Value::List { vals: val, .. } => val
+            Value::List { vals: val, .. } => format!(
-                .into_iter()
+                "[{}]",
-                .map(|x| x.collect_string())
+                val.into_iter()
-                .collect::<Vec<_>>()
+                    .map(|x| x.into_string(", "))
-                .join("\n"),
+                    .collect::<Vec<_>>()
-            Value::Record { vals, .. } => vals
+                    .join(separator)
-                .into_iter()
+            ),
-                .map(|y| y.collect_string())
+            Value::Record { cols, vals, .. } => format!(
-                .collect::<Vec<_>>()
+                "{{{}}}",
-                .join("\n"),
+                cols.iter()
+                    .zip(vals.iter())
+                    .map(|(x, y)| format!("{}: {}", x, y.clone().into_string(", ")))
+                    .collect::<Vec<_>>()
+                    .join(separator)
+            ),
             Value::Block { val, .. } => format!("<Block {}>", val),
             Value::Nothing { .. } => String::new(),
             Value::Error { error } => format!("{:?}", error),
             Value::Binary { val, .. } => format!("{:?}", val),
-            Value::CellPath { .. } => self.into_string(),
+            Value::CellPath { val, .. } => val.into_string(),
         }
     }
 
@@ -534,6 +545,15 @@ impl Value {
                 val: lhs.to_string() + rhs,
                 span,
             }),
+            (Value::Date { val: lhs, .. }, Value::Duration { val: rhs, .. }) => {
+                match lhs.checked_add_signed(chrono::Duration::nanoseconds(*rhs)) {
+                    Some(val) => Ok(Value::Date { val, span }),
+                    _ => Err(ShellError::OperatorOverflow(
+                        "addition operation overflowed".into(),
+                        span,
+                    )),
+                }
+            }
             (Value::Duration { val: lhs, .. }, Value::Duration { val: rhs, .. }) => {
                 if let Some(val) = lhs.checked_add(*rhs) {
                     Ok(Value::Duration { val, span })
@@ -590,6 +610,15 @@ impl Value {
                 val: lhs - rhs,
                 span,
             }),
+            (Value::Date { val: lhs, .. }, Value::Duration { val: rhs, .. }) => {
+                match lhs.checked_sub_signed(chrono::Duration::nanoseconds(*rhs)) {
+                    Some(val) => Ok(Value::Date { val, span }),
+                    _ => Err(ShellError::OperatorOverflow(
+                        "subtraction operation overflowed".into(),
+                        span,
+                    )),
+                }
+            }
             (Value::Duration { val: lhs, .. }, Value::Duration { val: rhs, .. }) => {
                 if let Some(val) = lhs.checked_sub(*rhs) {
                     Ok(Value::Duration { val, span })

crates/nu-protocol/src/value/row.rs

@@ -16,7 +16,7 @@ impl RowStream {
                 .join(", "),
             self.map(|x: Vec<Value>| {
                 x.into_iter()
-                    .map(|x| x.into_string())
+                    .map(|x| x.into_string(", "))
                     .collect::<Vec<String>>()
                     .join(", ")
             })

crates/nu-protocol/src/value/stream.rs

@@ -19,21 +19,15 @@ pub struct ValueStream {
 }
 
 impl ValueStream {
-    pub fn into_string(self) -> String {
+    pub fn into_string(self, separator: &str) -> String {
         format!(
             "[{}]",
-            self.map(|x: Value| x.into_string())
+            self.map(|x: Value| x.into_string(", "))
                 .collect::<Vec<String>>()
-                .join(", ")
+                .join(separator)
         )
     }
 
-    pub fn collect_string(self) -> String {
-        self.map(|x: Value| x.collect_string())
-            .collect::<Vec<String>>()
-            .join("\n")
-    }
-
     pub fn from_stream(
         input: impl Iterator<Item = Value> + Send + 'static,
         ctrlc: Option<Arc<AtomicBool>>,

src/main.rs

@@ -275,7 +275,7 @@ fn print_value(value: Value, engine_state: &EngineState) -> Result<(), ShellErro
             )?;
             table.collect_string()
         }
-        None => value.into_string(),
+        None => value.into_string(", "),
     };
     let stdout = std::io::stdout();