From b9419e0f3634dafc486fe3a8ee680e4105ceab1a Mon Sep 17 00:00:00 2001 From: Artemiy Date: Wed, 25 Jan 2023 17:42:53 +0300 Subject: [PATCH] To csv fix (#7850) # Description Fixes #7800 . `to csv` and `to tsv` no longer: - accept anything but records and tables as input, - accept lists that are not tables, - accept tables and records with values that are not primitives (other lists, tables and records). # User-Facing Changes Using `to csv` and `to tsv` on any of inputs mentioned above will result in `cant_convert` error. # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass # After Submitting If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. Co-authored-by: Stefan Holderbach --- crates/nu-command/src/formats/to/csv.rs | 10 +- crates/nu-command/src/formats/to/delimited.rs | 170 ++++++++++-------- crates/nu-command/src/formats/to/tsv.rs | 24 ++- .../tests/format_conversions/csv.rs | 38 ++++ 4 files changed, 162 insertions(+), 80 deletions(-) diff --git a/crates/nu-command/src/formats/to/csv.rs b/crates/nu-command/src/formats/to/csv.rs index 171778131f..fe5743c3ab 100644 --- a/crates/nu-command/src/formats/to/csv.rs +++ b/crates/nu-command/src/formats/to/csv.rs @@ -17,7 +17,10 @@ impl Command for ToCsv { fn signature(&self) -> Signature { Signature::build("to csv") - .input_output_types(vec![(Type::Any, Type::String)]) + .input_output_types(vec![ + (Type::Record(vec![]), Type::String), + (Type::Table(vec![]), Type::String), + ]) .named( "separator", SyntaxShape::String, @@ -44,6 +47,11 @@ impl Command for ToCsv { example: "[[foo bar]; [1 2]] | to csv -s ';' ", result: Some(Value::test_string("foo;bar\n1;2\n")), }, + Example { + description: "Outputs an CSV string representing the contents of this record", + example: "{a: 1 b: 2} | to csv", + result: Some(Value::test_string("a,b\n1,2\n")), + }, ] } diff --git a/crates/nu-command/src/formats/to/delimited.rs b/crates/nu-command/src/formats/to/delimited.rs index 117925e4e5..f66b4776cd 100644 --- a/crates/nu-command/src/formats/to/delimited.rs +++ b/crates/nu-command/src/formats/to/delimited.rs @@ -1,7 +1,8 @@ -use csv::WriterBuilder; +use csv::{Writer, WriterBuilder}; use indexmap::{indexset, IndexSet}; use nu_protocol::{Config, IntoPipelineData, PipelineData, ShellError, Span, Value}; use std::collections::VecDeque; +use std::error::Error; fn from_value_to_delimited_string( value: &Value, @@ -11,75 +12,92 @@ fn from_value_to_delimited_string( ) -> Result { match value { Value::Record { cols, vals, span } => { - let mut wtr = WriterBuilder::new() - .delimiter(separator as u8) - .from_writer(vec![]); - let mut fields: VecDeque = VecDeque::new(); - let mut values: VecDeque = VecDeque::new(); - - for (k, v) in cols.iter().zip(vals.iter()) { - fields.push_back(k.clone()); - - values.push_back(to_string_tagged_value(v, config, head, *span)?); - } - - wtr.write_record(fields).expect("can not write."); - wtr.write_record(values).expect("can not write."); - - let v = String::from_utf8(wtr.into_inner().map_err(|_| { - ShellError::CantConvert("record".to_string(), "string".to_string(), *span, None) - })?) - .map_err(|_| { - ShellError::CantConvert("record".to_string(), "string".to_string(), *span, None) - })?; - Ok(v) - } - Value::List { vals, span } => { - let mut wtr = WriterBuilder::new() - .delimiter(separator as u8) - .from_writer(vec![]); - - let merged_descriptors = merge_descriptors(vals); - - if merged_descriptors.is_empty() { - wtr.write_record( - vals.iter() - .map(|ele| { - to_string_tagged_value(ele, config, head, *span) - .unwrap_or_else(|_| String::new()) - }) - .collect::>(), - ) - .expect("can not write"); - } else { - wtr.write_record(merged_descriptors.iter().map(|item| &item[..])) - .expect("can not write."); - - for l in vals { - let mut row = vec![]; - for desc in &merged_descriptors { - row.push(match l.to_owned().get_data_by_key(desc) { - Some(s) => to_string_tagged_value(&s, config, head, *span)?, - None => String::new(), - }); - } - wtr.write_record(&row).expect("can not write"); - } - } - let v = String::from_utf8(wtr.into_inner().map_err(|_| { - ShellError::CantConvert("record".to_string(), "string".to_string(), *span, None) - })?) - .map_err(|_| { - ShellError::CantConvert("record".to_string(), "string".to_string(), *span, None) - })?; - Ok(v) + record_to_delimited(cols, vals, span, separator, config, head) } + Value::List { vals, span } => table_to_delimited(vals, span, separator, config, head), // Propagate errors by explicitly matching them before the final case. Value::Error { error } => Err(error.clone()), - other => to_string_tagged_value(value, config, other.expect_span(), head), + v => Err(make_unsupported_input_error(v, head, v.expect_span())), } } +fn record_to_delimited( + cols: &[String], + vals: &[Value], + span: &Span, + separator: char, + config: &Config, + head: Span, +) -> Result { + let mut wtr = WriterBuilder::new() + .delimiter(separator as u8) + .from_writer(vec![]); + let mut fields: VecDeque = VecDeque::new(); + let mut values: VecDeque = VecDeque::new(); + + for (k, v) in cols.iter().zip(vals.iter()) { + fields.push_back(k.clone()); + + values.push_back(to_string_tagged_value(v, config, head, *span)?); + } + + wtr.write_record(fields).expect("can not write."); + wtr.write_record(values).expect("can not write."); + + writer_to_string(wtr).map_err(|_| make_conversion_error("record", span)) +} + +fn table_to_delimited( + vals: &Vec, + span: &Span, + separator: char, + config: &Config, + head: Span, +) -> Result { + if let Some(val) = find_non_record(vals) { + return Err(make_unsupported_input_error(val, head, *span)); + } + + let mut wtr = WriterBuilder::new() + .delimiter(separator as u8) + .from_writer(vec![]); + + let merged_descriptors = merge_descriptors(vals); + + if merged_descriptors.is_empty() { + let vals = vals + .iter() + .map(|ele| { + to_string_tagged_value(ele, config, head, *span).unwrap_or_else(|_| String::new()) + }) + .collect::>(); + wtr.write_record(vals).expect("can not write"); + } else { + wtr.write_record(merged_descriptors.iter().map(|item| &item[..])) + .expect("can not write."); + + for l in vals { + let mut row = vec![]; + for desc in &merged_descriptors { + row.push(match l.to_owned().get_data_by_key(desc) { + Some(s) => to_string_tagged_value(&s, config, head, *span)?, + None => String::new(), + }); + } + wtr.write_record(&row).expect("can not write"); + } + } + writer_to_string(wtr).map_err(|_| make_conversion_error("table", span)) +} + +fn writer_to_string(writer: Writer>) -> Result> { + Ok(String::from_utf8(writer.into_inner()?)?) +} + +fn make_conversion_error(type_from: &str, span: &Span) -> ShellError { + ShellError::CantConvert(type_from.to_string(), "string".to_string(), *span, None) +} + fn to_string_tagged_value( v: &Value, config: &Config, @@ -95,22 +113,30 @@ fn to_string_tagged_value( | Value::CustomValue { .. } | Value::Filesize { .. } | Value::CellPath { .. } - | Value::List { .. } - | Value::Record { .. } | Value::Float { .. } => Ok(v.clone().into_abbreviated_string(config)), Value::Date { val, .. } => Ok(val.to_string()), Value::Nothing { .. } => Ok(String::new()), // Propagate existing errors Value::Error { error } => Err(error.clone()), - _ => Err(ShellError::UnsupportedInput( - "Unexpected type".to_string(), - format!("input type: {:?}", v.get_type()), - head, - span, - )), + _ => Err(make_unsupported_input_error(v, head, span)), } } +fn make_unsupported_input_error(value: &Value, head: Span, span: Span) -> ShellError { + ShellError::UnsupportedInput( + "Unexpected type".to_string(), + format!("input type: {:?}", value.get_type()), + head, + span, + ) +} + +pub fn find_non_record(values: &[Value]) -> Option<&Value> { + values + .iter() + .find(|val| !matches!(val, Value::Record { .. })) +} + pub fn merge_descriptors(values: &[Value]) -> Vec { let mut ret: Vec = vec![]; let mut seen: IndexSet = indexset! {}; diff --git a/crates/nu-command/src/formats/to/tsv.rs b/crates/nu-command/src/formats/to/tsv.rs index a4b3f59b6b..edcb2ab7fc 100644 --- a/crates/nu-command/src/formats/to/tsv.rs +++ b/crates/nu-command/src/formats/to/tsv.rs @@ -15,7 +15,10 @@ impl Command for ToTsv { fn signature(&self) -> Signature { Signature::build("to tsv") - .input_output_types(vec![(Type::Any, Type::String)]) + .input_output_types(vec![ + (Type::Record(vec![]), Type::String), + (Type::Table(vec![]), Type::String), + ]) .switch( "noheaders", "do not output the column names as the first row", @@ -29,11 +32,18 @@ impl Command for ToTsv { } fn examples(&self) -> Vec { - vec![Example { - description: "Outputs an TSV string representing the contents of this table", - example: "[[foo bar]; [1 2]] | to tsv", - result: Some(Value::test_string("foo\tbar\n1\t2\n")), - }] + vec![ + Example { + description: "Outputs an TSV string representing the contents of this table", + example: "[[foo bar]; [1 2]] | to tsv", + result: Some(Value::test_string("foo\tbar\n1\t2\n")), + }, + Example { + description: "Outputs an TSV string representing the contents of this record", + example: "{a: 1 b: 2} | to tsv", + result: Some(Value::test_string("a\tb\n1\t2\n")), + }, + ] } fn run( @@ -42,7 +52,7 @@ impl Command for ToTsv { _stack: &mut Stack, call: &Call, input: PipelineData, - ) -> Result { + ) -> Result { let head = call.head; let noheaders = call.has_flag("noheaders"); let config = engine_state.get_config(); diff --git a/crates/nu-command/tests/format_conversions/csv.rs b/crates/nu-command/tests/format_conversions/csv.rs index 2a8736175a..6c1ab564b5 100644 --- a/crates/nu-command/tests/format_conversions/csv.rs +++ b/crates/nu-command/tests/format_conversions/csv.rs @@ -207,3 +207,41 @@ fn from_csv_text_skipping_headers_to_table() { assert_eq!(actual.out, "3"); }) } + +#[test] +fn table_with_record_error() { + let actual = nu!( + cwd: "tests/fixtures/formats", pipeline( + r#" + [[a b]; [1 2] [3 {a: 1 b: 2}]] + | to csv + "# + )); + + assert!(actual.err.contains("can't convert")) +} + +#[test] +fn list_not_table_error() { + let actual = nu!( + cwd: "tests/fixtures/formats", pipeline( + r#" + [{a: 1 b: 2} {a: 3 b: 4} 1] + | to csv + "# + )); + + assert!(actual.err.contains("can't convert")) +} + +#[test] +fn string_to_csv_error() { + let actual = nu!( + cwd: "tests/fixtures/formats", pipeline( + r#" + 'qwe' | to csv + "# + )); + + assert!(actual.err.contains("can't convert")) +}