From 299d199150e2974ad22e415afcde6fede14d768b Mon Sep 17 00:00:00 2001 From: Darren Schroeder <343840+fdncred@users.noreply.github.com> Date: Thu, 17 Oct 2024 16:14:01 -0500 Subject: [PATCH] allow `group-by` and `split-by` to work with other values (#14086) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description This PR updates `group-by` and `split-by` to allow other nushell Values to be used, namely bools. ### Before ```nushell ❯ [false, false, true, false, true, false] | group-by | table -e Error: nu::shell::cant_convert × Can't convert to string. ╭─[entry #1:1:2] 1 │ [false, false, true, false, true, false] | group-by | table -e · ──┬── · ╰── can't convert bool to string ╰──── ``` ### After ```nushell ❯ [false, false, true, false, true, false] | group-by | table -e ╭───────┬───────────────╮ │ │ ╭───┬───────╮ │ │ false │ │ 0 │ false │ │ │ │ │ 1 │ false │ │ │ │ │ 2 │ false │ │ │ │ │ 3 │ false │ │ │ │ ╰───┴───────╯ │ │ │ ╭───┬──────╮ │ │ true │ │ 0 │ true │ │ │ │ │ 1 │ true │ │ │ │ ╰───┴──────╯ │ ╰───────┴───────────────╯ ``` # User-Facing Changes # Tests + Formatting # After Submitting --- crates/nu-command/src/filters/group_by.rs | 40 ++++++++++++--- crates/nu-command/src/filters/split_by.rs | 21 ++++---- crates/nu-command/tests/commands/group_by.rs | 52 +++++++++----------- 3 files changed, 70 insertions(+), 43 deletions(-) diff --git a/crates/nu-command/src/filters/group_by.rs b/crates/nu-command/src/filters/group_by.rs index 78789c606a..74a19c38e3 100644 --- a/crates/nu-command/src/filters/group_by.rs +++ b/crates/nu-command/src/filters/group_by.rs @@ -38,6 +38,14 @@ impl Command for GroupBy { "Splits a list or table into groups, and returns a record containing those groups." } + fn extra_description(&self) -> &str { + r#"the group-by command makes some assumptions: + - if the input data is not a string, the grouper will convert the key to string but the values will remain in their original format. e.g. with bools, "true" and true would be in the same group (see example). + - datetime is formatted based on your configuration setting. use `format date` to change the format. + - filesize is formatted based on your configuration setting. use `format filesize` to change the format. + - some nushell values are not supported, such as closures."# + } + fn run( &self, engine_state: &EngineState, @@ -114,6 +122,20 @@ impl Command for GroupBy { }), ])), }, + Example { + description: "Group bools, whether they are strings or actual bools", + example: r#"[true "true" false "false"] | group-by"#, + result: Some(Value::test_record(record! { + "true" => Value::test_list(vec![ + Value::test_bool(true), + Value::test_string("true"), + ]), + "false" => Value::test_list(vec![ + Value::test_bool(false), + Value::test_string("false"), + ]), + })), + } ] } } @@ -127,6 +149,7 @@ pub fn group_by( let head = call.head; let grouper: Option = call.opt(engine_state, stack, 0)?; let to_table = call.has_flag(engine_state, stack, "to-table")?; + let config = engine_state.get_config(); let values: Vec = input.into_iter().collect(); if values.is_empty() { @@ -137,7 +160,7 @@ pub fn group_by( Some(grouper) => { let span = grouper.span(); match grouper { - Value::CellPath { val, .. } => group_cell_path(val, values)?, + Value::CellPath { val, .. } => group_cell_path(val, values, config)?, Value::Closure { val, .. } => { group_closure(values, span, *val, engine_state, stack)? } @@ -149,7 +172,7 @@ pub fn group_by( } } } - None => group_no_grouper(values)?, + None => group_no_grouper(values, config)?, }; let value = if to_table { @@ -164,6 +187,7 @@ pub fn group_by( fn group_cell_path( column_name: CellPath, values: Vec, + config: &nu_protocol::Config, ) -> Result>, ShellError> { let mut groups = IndexMap::<_, Vec<_>>::new(); @@ -176,18 +200,21 @@ fn group_cell_path( continue; // likely the result of a failed optional access, ignore this value } - let key = key.coerce_string()?; + let key = key.to_abbreviated_string(config); groups.entry(key).or_default().push(value); } Ok(groups) } -fn group_no_grouper(values: Vec) -> Result>, ShellError> { +fn group_no_grouper( + values: Vec, + config: &nu_protocol::Config, +) -> Result>, ShellError> { let mut groups = IndexMap::<_, Vec<_>>::new(); for value in values.into_iter() { - let key = value.coerce_string()?; + let key = value.to_abbreviated_string(config); groups.entry(key).or_default().push(value); } @@ -203,12 +230,13 @@ fn group_closure( ) -> Result>, ShellError> { let mut groups = IndexMap::<_, Vec<_>>::new(); let mut closure = ClosureEval::new(engine_state, stack, closure); + let config = engine_state.get_config(); for value in values { let key = closure .run_with_value(value.clone())? .into_value(span)? - .coerce_into_string()?; + .to_abbreviated_string(config); groups.entry(key).or_default().push(value); } diff --git a/crates/nu-command/src/filters/split_by.rs b/crates/nu-command/src/filters/split_by.rs index 7a5898e608..217918cf91 100644 --- a/crates/nu-command/src/filters/split_by.rs +++ b/crates/nu-command/src/filters/split_by.rs @@ -84,16 +84,16 @@ pub fn split_by( input: PipelineData, ) -> Result { let name = call.head; - + let config = engine_state.get_config(); let splitter: Option = call.opt(engine_state, stack, 0)?; match splitter { Some(v) => { let splitter = Some(Spanned { - item: v.coerce_into_string()?, + item: v.to_abbreviated_string(config), span: name, }); - Ok(split(splitter.as_ref(), input, name)?) + Ok(split(splitter.as_ref(), input, name, config)?) } // This uses the same format as the 'requires a column name' error in sort_utils.rs None => Err(ShellError::GenericError { @@ -110,6 +110,7 @@ pub fn split( column_name: Option<&Spanned>, values: PipelineData, span: Span, + config: &nu_protocol::Config, ) -> Result { let grouper = if let Some(column_name) = column_name { Grouper::ByColumn(Some(column_name.clone())) @@ -127,7 +128,7 @@ pub fn split( }; match group_key { - Some(group_key) => Ok(group_key.coerce_string()?), + Some(group_key) => Ok(group_key.to_abbreviated_string(config)), None => Err(ShellError::CantFindColumn { col_name: column_name.item.to_string(), span: Some(column_name.span), @@ -136,12 +137,12 @@ pub fn split( } }; - data_split(values, Some(&block), span) + data_split(values, Some(&block), span, config) } Grouper::ByColumn(None) => { - let block = move |_, row: &Value| row.coerce_string(); + let block = move |_, row: &Value| Ok(row.to_abbreviated_string(config)); - data_split(values, Some(&block), span) + data_split(values, Some(&block), span, config) } } } @@ -151,6 +152,7 @@ fn data_group( values: &Value, grouper: Option<&dyn Fn(usize, &Value) -> Result>, span: Span, + config: &nu_protocol::Config, ) -> Result { let mut groups: IndexMap> = IndexMap::new(); @@ -158,7 +160,7 @@ fn data_group( let group_key = if let Some(ref grouper) = grouper { grouper(idx, &value) } else { - value.coerce_string() + Ok(value.to_abbreviated_string(config)) }; let group = groups.entry(group_key?).or_default(); @@ -179,6 +181,7 @@ pub fn data_split( value: PipelineData, splitter: Option<&dyn Fn(usize, &Value) -> Result>, dst_span: Span, + config: &nu_protocol::Config, ) -> Result { let mut splits = indexmap::IndexMap::new(); @@ -188,7 +191,7 @@ pub fn data_split( match v { Value::Record { val: grouped, .. } => { for (outer_key, list) in grouped.into_owned() { - match data_group(&list, splitter, span) { + match data_group(&list, splitter, span, config) { Ok(grouped_vals) => { if let Value::Record { val: sub, .. } = grouped_vals { for (inner_key, subset) in sub.into_owned() { diff --git a/crates/nu-command/tests/commands/group_by.rs b/crates/nu-command/tests/commands/group_by.rs index c6d0903c94..232bbcba0f 100644 --- a/crates/nu-command/tests/commands/group_by.rs +++ b/crates/nu-command/tests/commands/group_by.rs @@ -23,37 +23,33 @@ fn groups() { #[test] fn errors_if_given_unknown_column_name() { - let sample = r#" - { - "nu": { - "committers": [ - {"name": "Andrés N. Robalino"}, - {"name": "JT Turner"}, - {"name": "Yehuda Katz"} - ], - "releases": [ - {"version": "0.2"} - {"version": "0.8"}, - {"version": "0.9999999"} - ], - "0xATYKARNU": [ - ["Th", "e", " "], - ["BIG", " ", "UnO"], - ["punto", "cero"] - ] - } - } - "#; + let sample = r#"{ + "nu": { + "committers": [ + {"name": "Andrés N. Robalino"}, + {"name": "JT Turner"}, + {"name": "Yehuda Katz"} + ], + "releases": [ + {"version": "0.2"} + {"version": "0.8"}, + {"version": "0.9999999"} + ], + "0xATYKARNU": [ + ["Th", "e", " "], + ["BIG", " ", "UnO"], + ["punto", "cero"] + ] + } +} +"#; let actual = nu!(pipeline(&format!( - r#" - '{sample}' - | from json - | group-by {{|| get nu.releases.version }} - "# + r#"'{sample}' + | from json + | group-by {{|| get nu.releases.missing_column }}"# ))); - - assert!(actual.err.contains("can't convert list to string")); + assert!(actual.err.contains("cannot find column")); } #[test]