From 62198a29c2d0aee90f92ba776cfd01acf21b7ee2 Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Tue, 5 Nov 2024 00:33:54 -0800 Subject: [PATCH] Make `to text` line endings consistent for list (streams) (#14166) # Description Fixes #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 | 68 ++++++++++++++------- crates/nu-command/tests/commands/to_text.rs | 57 +++++++++++++---- 2 files changed, 93 insertions(+), 32 deletions(-) diff --git a/crates/nu-command/src/formats/to/text.rs b/crates/nu-command/src/formats/to/text.rs index 3b365d6649..b464cb86aa 100644 --- a/crates/nu-command/src/formats/to/text.rs +++ b/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 { - 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( - ByteStream::from_iter( - iter, + 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, - ), - update_metadata(meta), - )) + 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( + 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, + ) + }; + + 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 { 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")), }, diff --git a/crates/nu-command/tests/commands/to_text.rs b/crates/nu-command/tests/commands/to_text.rs index f51e3838fd..0e5873be5a 100644 --- a/crates/nu-command/tests/commands/to_text.rs +++ b/crates/nu-command/tests/commands/to_text.rs @@ -1,19 +1,54 @@ use nu_test_support::nu; -#[test] -fn list_to_text() { - let actual = nu!(r#"["foo" "bar" "baz"] | to text"#); +const LINE_LEN: usize = if cfg!(target_os = "windows") { 2 } else { 1 }; - // these actually have newlines between them in the real world but nu! strips newlines, grr - assert_eq!(actual.out, "foobarbaz"); +#[test] +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 +// 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_stream() { + let actual = nu!(r#"[] | each {} | to text | str length"#); + assert_eq!(actual.out, "0"); - // 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] | each {} | to text | str length"#); + assert_eq!(actual.out, (1 + LINE_LEN).to_string()); + + let actual = nu!(r#"[a b] | each {} | to text | str length"#); + assert_eq!(actual.out, (2 * (1 + LINE_LEN)).to_string()); +} + +#[test] +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"); + + let actual = nu!(r#"[a b] | each {} | to text -n | str length"#); + assert_eq!(actual.out, (2 + LINE_LEN).to_string()); }