Make to text line endings consistent for list (streams) (nushell#14166

)

# Description
Fixes nushell#14151 where `to text` treats list streams and lists values
differently.

# User-Facing Changes
New line is always added after items in a list or record except for the
last item if the `--no-newline` flag is provided.

crates/nu-command/src/formats/to/text.rs

@@ -3,6 +3,7 @@ use nu_engine::command_prelude::*;
 use nu_protocol::{
     format_duration, format_filesize_from_conf, ByteStream, Config, PipelineMetadata,
 };
+use std::io::Write;
 
 const LINE_ENDING: &str = if cfg!(target_os = "windows") {
     "\r\n"
@@ -40,44 +41,69 @@ impl Command for ToText {
         call: &Call,
         input: PipelineData,
     ) -> Result<PipelineData, ShellError> {
-        let span = call.head;
+        let head = call.head;
         let no_newline = call.has_flag(engine_state, stack, "no-newline")?;
         let input = input.try_expand_range()?;
         let config = stack.get_config(engine_state);
 
         match input {
-            PipelineData::Empty => Ok(Value::string(String::new(), span)
+            PipelineData::Empty => Ok(Value::string(String::new(), head)
                 .into_pipeline_data_with_metadata(update_metadata(None))),
             PipelineData::Value(value, ..) => {
-                let is_compound_type = matches!(value, Value::List { .. } | Value::Record { .. });
+                let add_trailing = !no_newline
+                    && match &value {
+                        Value::List { vals, .. } => !vals.is_empty(),
+                        Value::Record { val, .. } => !val.is_empty(),
+                        _ => false,
+                    };
                 let mut str = local_into_string(value, LINE_ENDING, &config);
-                if is_compound_type && !no_newline {
+                if add_trailing {
                     str.push_str(LINE_ENDING);
                 }
-
                 Ok(
-                    Value::string(str, span)
+                    Value::string(str, head)
                         .into_pipeline_data_with_metadata(update_metadata(None)),
                 )
             }
             PipelineData::ListStream(stream, meta) => {
                 let span = stream.span();
-                let iter = stream.into_inner().map(move |value| {
-                    let mut str = local_into_string(value, LINE_ENDING, &config);
-                    if !no_newline {
-                        str.push_str(LINE_ENDING);
-                    }
-                    str
-                });
-                Ok(PipelineData::ByteStream(
+                let stream = if no_newline {
+                    let mut first = true;
+                    let mut iter = stream.into_inner();
+                    ByteStream::from_fn(
+                        span,
+                        engine_state.signals().clone(),
+                        ByteStreamType::String,
+                        move |buf| {
+                            let Some(val) = iter.next() else {
+                                return Ok(false);
+                            };
+                            if first {
+                                first = false;
+                            } else {
+                                write!(buf, "{LINE_ENDING}").err_span(head)?;
+                            }
+                            // TODO: write directly into `buf` instead of creating an intermediate
+                            // string.
+                            let str = local_into_string(val, LINE_ENDING, &config);
+                            write!(buf, "{str}").err_span(head)?;
+                            Ok(true)
+                        },
+                    )
+                } else {
                     ByteStream::from_iter(
-                        iter,
+                        stream.into_inner().map(move |val| {
+                            let mut str = local_into_string(val, LINE_ENDING, &config);
+                            str.push_str(LINE_ENDING);
+                            str
+                        }),
                         span,
                         engine_state.signals().clone(),
                         ByteStreamType::String,
-                    ),
-                    update_metadata(meta),
-                ))
+                    )
+                };
+
+                Ok(PipelineData::ByteStream(stream, update_metadata(meta)))
             }
             PipelineData::ByteStream(stream, meta) => {
                 Ok(PipelineData::ByteStream(stream, update_metadata(meta)))
@@ -88,12 +114,12 @@ impl Command for ToText {
     fn examples(&self) -> Vec<Example> {
         vec![
             Example {
-                description: "Outputs data as simple text with a newline",
+                description: "Outputs data as simple text with a trailing newline",
                 example: "[1] | to text",
                 result: Some(Value::test_string("1".to_string() + LINE_ENDING)),
             },
             Example {
-                description: "Outputs data as simple text without a newline",
+                description: "Outputs data as simple text without a trailing newline",
                 example: "[1] | to text --no-newline",
                 result: Some(Value::test_string("1")),
             },

crates/nu-command/tests/commands/to_text.rs

@@ -1,19 +1,54 @@
 use nu_test_support::nu;
 
+const LINE_LEN: usize = if cfg!(target_os = "windows") { 2 } else { 1 };
+
 #[test]
-fn list_to_text() {
-    let actual = nu!(r#"["foo" "bar" "baz"] | to text"#);
+fn list() {
+    // Using `str length` since nu! strips newlines, grr
+    let actual = nu!(r#"[] | to text | str length"#);
+    assert_eq!(actual.out, "0");
+
+    let actual = nu!(r#"[a] | to text | str length"#);
+    assert_eq!(actual.out, (1 + LINE_LEN).to_string());
+
+    let actual = nu!(r#"[a b] | to text | str length"#);
+    assert_eq!(actual.out, (2 * (1 + LINE_LEN)).to_string());
+}
+
+// The output should be the same when `to text` gets a ListStream instead of a Value::List.
+#[test]
+fn list_stream() {
+    let actual = nu!(r#"[] | each {} | to text | str length"#);
+    assert_eq!(actual.out, "0");
+
+    let actual = nu!(r#"[a] | each {} | to text | str length"#);
+    assert_eq!(actual.out, (1 + LINE_LEN).to_string());
 
-    // these actually have newlines between them in the real world but nu! strips newlines, grr
-    assert_eq!(actual.out, "foobarbaz");
+    let actual = nu!(r#"[a b] | each {} | to text | str length"#);
+    assert_eq!(actual.out, (2 * (1 + LINE_LEN)).to_string());
 }
 
-// the output should be the same when `to text` gets a ListStream instead of a Value::List
 #[test]
-fn list_stream_to_text() {
-    // use `each` to convert the list to a ListStream
-    let actual = nu!(r#"["foo" "bar" "baz"] | each {|i| $i} | to text"#);
+fn list_no_newline() {
+    let actual = nu!(r#"[] | to text -n | str length"#);
+    assert_eq!(actual.out, "0");
+
+    let actual = nu!(r#"[a] | to text -n | str length"#);
+    assert_eq!(actual.out, "1");
+
+    let actual = nu!(r#"[a b] | to text -n | str length"#);
+    assert_eq!(actual.out, (2 + LINE_LEN).to_string());
+}
+
+// The output should be the same when `to text` gets a ListStream instead of a Value::List.
+#[test]
+fn list_stream_no_newline() {
+    let actual = nu!(r#"[] | each {} | to text -n | str length"#);
+    assert_eq!(actual.out, "0");
+
+    let actual = nu!(r#"[a] | each {} | to text -n | str length"#);
+    assert_eq!(actual.out, "1");
 
-    // these actually have newlines between them in the real world but nu! strips newlines, grr
-    assert_eq!(actual.out, "foobarbaz");
+    let actual = nu!(r#"[a b] | each {} | to text -n | str length"#);
+    assert_eq!(actual.out, (2 + LINE_LEN).to_string());
 }