From 15f3a545f02cb660e91bf2e995981f474b31d290 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radek=20V=C3=ADt?= Date: Fri, 18 Sep 2020 08:40:20 +0200 Subject: [PATCH] Cleanup code in get and nu-value-ext (#2563) * Cleanup code in get and nu-value-ext * Remove unnecessary return statements from get --- crates/nu-cli/src/commands/get.rs | 364 ++++++++++++------------ crates/nu-cli/src/commands/histogram.rs | 6 +- crates/nu-cli/src/commands/select.rs | 4 +- crates/nu-data/src/base.rs | 4 +- crates/nu-value-ext/src/lib.rs | 41 +-- crates/nu_plugin_inc/src/inc.rs | 34 +-- 6 files changed, 234 insertions(+), 219 deletions(-) diff --git a/crates/nu-cli/src/commands/get.rs b/crates/nu-cli/src/commands/get.rs index 5474e8703d..1a28a667a4 100644 --- a/crates/nu-cli/src/commands/get.rs +++ b/crates/nu-cli/src/commands/get.rs @@ -4,8 +4,8 @@ use indexmap::set::IndexSet; use log::trace; use nu_errors::ShellError; use nu_protocol::{ - did_you_mean, ColumnPath, PathMember, Primitive, ReturnSuccess, Signature, SyntaxShape, - UnspannedPathMember, UntaggedValue, Value, + did_you_mean, ColumnPath, Dictionary, PathMember, Primitive, ReturnSuccess, Signature, + SyntaxShape, UnspannedPathMember, UntaggedValue, Value, }; use nu_source::HasFallibleSpan; use nu_value_ext::get_data_by_column_path; @@ -58,195 +58,209 @@ impl WholeStreamCommand for Get { } } -pub fn get_column_path(path: &ColumnPath, obj: &Value) -> Result { - let fields = path.clone(); - - get_data_by_column_path( - obj, - path, - Box::new(move |(obj_source, column_path_tried, error)| { - let path_members_span = fields.maybe_span().unwrap_or_else(Span::unknown); - - match &obj_source.value { - UntaggedValue::Table(rows) => match column_path_tried { - PathMember { - unspanned: UnspannedPathMember::String(column), - .. - } => { - let primary_label = format!("There isn't a column named '{}'", &column); - - let suggestions: IndexSet<_> = rows - .iter() - .filter_map(|r| did_you_mean(&r, &column_path_tried)) - .map(|s| s[0].1.to_owned()) - .collect(); - let mut existing_columns: IndexSet<_> = IndexSet::default(); - let mut names: Vec = vec![]; - - for row in rows { - for field in row.data_descriptors() { - if !existing_columns.contains(&field[..]) { - existing_columns.insert(field.clone()); - names.push(field); - } - } - } - - if names.is_empty() { - return ShellError::labeled_error_with_secondary( - "Unknown column", - primary_label, - column_path_tried.span, - "Appears to contain rows. Try indexing instead.", - column_path_tried.span.since(path_members_span), - ); - } else { - return ShellError::labeled_error_with_secondary( - "Unknown column", - primary_label, - column_path_tried.span, - format!( - "Perhaps you meant '{}'? Columns available: {}", - suggestions - .iter() - .map(|x| x.to_owned()) - .collect::>() - .join(","), - names.join(",") - ), - column_path_tried.span.since(path_members_span), - ); - }; - } - PathMember { - unspanned: UnspannedPathMember::Int(idx), - .. - } => { - let total = rows.len(); - - let secondary_label = if total == 1 { - "The table only has 1 row".to_owned() - } else { - format!("The table only has {} rows (0 to {})", total, total - 1) - }; - - return ShellError::labeled_error_with_secondary( - "Row not found", - format!("There isn't a row indexed at {}", idx), - column_path_tried.span, - secondary_label, - column_path_tried.span.since(path_members_span), - ); - } - }, - UntaggedValue::Row(columns) => match column_path_tried { - PathMember { - unspanned: UnspannedPathMember::String(column), - .. - } => { - let primary_label = format!("There isn't a column named '{}'", &column); - - if let Some(suggestions) = did_you_mean(&obj_source, column_path_tried) { - return ShellError::labeled_error_with_secondary( - "Unknown column", - primary_label, - column_path_tried.span, - format!( - "Perhaps you meant '{}'? Columns available: {}", - suggestions[0].1, - &obj_source.data_descriptors().join(",") - ), - column_path_tried.span.since(path_members_span), - ); - } - } - PathMember { - unspanned: UnspannedPathMember::Int(idx), - .. - } => { - return ShellError::labeled_error_with_secondary( - "No rows available", - format!("A row at '{}' can't be indexed.", &idx), - column_path_tried.span, - format!( - "Appears to contain columns. Columns available: {}", - columns.keys().join(",") - ), - column_path_tried.span.since(path_members_span), - ) - } - }, - _ => {} - } - - if let Some(suggestions) = did_you_mean(&obj_source, column_path_tried) { - return ShellError::labeled_error( - "Unknown column", - format!("did you mean '{}'?", suggestions[0].1), - column_path_tried.span.since(path_members_span), - ); - } - - error - }), - ) -} - pub async fn get( args: CommandArgs, registry: &CommandRegistry, ) -> Result { let registry = registry.clone(); - let (GetArgs { rest: mut fields }, mut input) = args.process(®istry).await?; - if fields.is_empty() { + let (GetArgs { rest: column_paths }, mut input) = args.process(®istry).await?; + if column_paths.is_empty() { let vec = input.drain_vec().await; let descs = nu_protocol::merge_descriptors(&vec); Ok(futures::stream::iter(descs.into_iter().map(ReturnSuccess::value)).to_output_stream()) } else { - let member = fields.remove(0); - trace!("get {:?} {:?}", member, fields); - - Ok(input + trace!("get {:?}", column_paths); + let output_stream = input .map(move |item| { - let member = vec![member.clone()]; - - let column_paths = vec![&member, &fields] - .into_iter() + let output = column_paths + .iter() + .map(move |path| get_output(&item, path)) .flatten() - .collect::>(); - - let mut output = vec![]; - for path in column_paths { - let res = get_column_path(&path, &item); - - match res { - Ok(got) => match got { - Value { - value: UntaggedValue::Table(rows), - .. - } => { - for item in rows { - output.push(ReturnSuccess::value(item.clone())); - } - } - Value { - value: UntaggedValue::Primitive(Primitive::Nothing), - .. - } => {} - other => output.push(ReturnSuccess::value(other.clone())), - }, - Err(reason) => output.push(ReturnSuccess::value( - UntaggedValue::Error(reason).into_untagged_value(), - )), - } - } - + .collect::>(); futures::stream::iter(output) }) .flatten() - .to_output_stream()) + .to_output_stream(); + Ok(output_stream) + } +} + +fn get_output(item: &Value, path: &ColumnPath) -> Vec> { + match get_column_path(path, item) { + Ok(Value { + value: UntaggedValue::Primitive(Primitive::Nothing), + .. + }) => vec![], + Ok(Value { + value: UntaggedValue::Table(rows), + .. + }) => rows.into_iter().map(ReturnSuccess::value).collect(), + Ok(other) => vec![ReturnSuccess::value(other)], + Err(reason) => vec![ReturnSuccess::value( + UntaggedValue::Error(reason).into_untagged_value(), + )], + } +} + +pub fn get_column_path(path: &ColumnPath, obj: &Value) -> Result { + get_data_by_column_path(obj, path, move |obj_source, column_path_tried, error| { + let path_members_span = path.maybe_span().unwrap_or_else(Span::unknown); + + match &obj_source.value { + UntaggedValue::Table(rows) => { + return get_column_path_from_table_error( + rows, + column_path_tried, + &path_members_span, + ); + } + UntaggedValue::Row(columns) => { + if let Some(error) = get_column_from_row_error( + columns, + column_path_tried, + &path_members_span, + obj_source, + ) { + return error; + } + } + _ => {} + } + + if let Some(suggestions) = did_you_mean(&obj_source, column_path_tried) { + ShellError::labeled_error( + "Unknown column", + format!("did you mean '{}'?", suggestions[0].1), + column_path_tried.span.since(path_members_span), + ) + } else { + error + } + }) +} + +pub fn get_column_path_from_table_error( + rows: &[Value], + column_path_tried: &PathMember, + path_members_span: &Span, +) -> ShellError { + match column_path_tried { + PathMember { + unspanned: UnspannedPathMember::String(column), + .. + } => { + let primary_label = format!("There isn't a column named '{}'", &column); + + let suggestions: IndexSet<_> = rows + .iter() + .filter_map(|r| did_you_mean(&r, &column_path_tried)) + .map(|s| s[0].1.to_owned()) + .collect(); + let mut existing_columns: IndexSet<_> = IndexSet::default(); + let mut names: Vec = vec![]; + + for row in rows { + for field in row.data_descriptors() { + if !existing_columns.contains(&field[..]) { + existing_columns.insert(field.clone()); + names.push(field); + } + } + } + + if names.is_empty() { + ShellError::labeled_error_with_secondary( + "Unknown column", + primary_label, + column_path_tried.span, + "Appears to contain rows. Try indexing instead.", + column_path_tried.span.since(path_members_span), + ) + } else { + ShellError::labeled_error_with_secondary( + "Unknown column", + primary_label, + column_path_tried.span, + format!( + "Perhaps you meant '{}'? Columns available: {}", + suggestions + .iter() + .map(|x| x.to_owned()) + .collect::>() + .join(","), + names.join(",") + ), + column_path_tried.span.since(path_members_span), + ) + } + } + PathMember { + unspanned: UnspannedPathMember::Int(idx), + .. + } => { + let total = rows.len(); + + let secondary_label = if total == 1 { + "The table only has 1 row".to_owned() + } else { + format!("The table only has {} rows (0 to {})", total, total - 1) + }; + + ShellError::labeled_error_with_secondary( + "Row not found", + format!("There isn't a row indexed at {}", idx), + column_path_tried.span, + secondary_label, + column_path_tried.span.since(path_members_span), + ) + } + } +} + +pub fn get_column_from_row_error( + columns: &Dictionary, + column_path_tried: &PathMember, + path_members_span: &Span, + obj_source: &Value, +) -> Option { + match column_path_tried { + PathMember { + unspanned: UnspannedPathMember::String(column), + .. + } => { + let primary_label = format!("There isn't a column named '{}'", &column); + + if let Some(suggestions) = did_you_mean(&obj_source, column_path_tried) { + Some(ShellError::labeled_error_with_secondary( + "Unknown column", + primary_label, + column_path_tried.span, + format!( + "Perhaps you meant '{}'? Columns available: {}", + suggestions[0].1, + &obj_source.data_descriptors().join(",") + ), + column_path_tried.span.since(path_members_span), + )) + } else { + None + } + } + PathMember { + unspanned: UnspannedPathMember::Int(idx), + .. + } => Some(ShellError::labeled_error_with_secondary( + "No rows available", + format!("A row at '{}' can't be indexed.", &idx), + column_path_tried.span, + format!( + "Appears to contain columns. Columns available: {}", + columns.keys().join(",") + ), + column_path_tried.span.since(path_members_span), + )), } } diff --git a/crates/nu-cli/src/commands/histogram.rs b/crates/nu-cli/src/commands/histogram.rs index ae1d94457c..902f9f33a5 100644 --- a/crates/nu-cli/src/commands/histogram.rs +++ b/crates/nu-cli/src/commands/histogram.rs @@ -178,11 +178,7 @@ fn evaluator(by: ColumnPath) -> Box Result Ok(with_value), diff --git a/crates/nu-cli/src/commands/select.rs b/crates/nu-cli/src/commands/select.rs index c6920a9aba..05616906d8 100644 --- a/crates/nu-cli/src/commands/select.rs +++ b/crates/nu-cli/src/commands/select.rs @@ -83,7 +83,7 @@ async fn select(args: CommandArgs, registry: &CommandRegistry) -> Result Result impl FnOnce((&Value, &PathMember, ShellError)) -> ShellError { - move |(_obj_source, _column_path_tried, _err)| ShellError::unimplemented(reason) + ) -> impl FnOnce(&Value, &PathMember, ShellError) -> ShellError { + move |_obj_source, _column_path_tried, _err| ShellError::unimplemented(reason) } fn column_path(paths: &[Value]) -> Result, ShellError> { diff --git a/crates/nu-value-ext/src/lib.rs b/crates/nu-value-ext/src/lib.rs index f446bb478c..ed9bc7e873 100644 --- a/crates/nu-value-ext/src/lib.rs +++ b/crates/nu-value-ext/src/lib.rs @@ -18,7 +18,7 @@ pub trait ValueExt { fn get_data_by_column_path( &self, path: &ColumnPath, - callback: Box ShellError>, + callback: Box ShellError>, ) -> Result; fn swap_data_by_column_path( &self, @@ -66,9 +66,9 @@ impl ValueExt for Value { fn get_data_by_column_path( &self, path: &ColumnPath, - callback: Box ShellError>, + get_error: Box ShellError>, ) -> Result { - get_data_by_column_path(self, path, callback) + get_data_by_column_path(self, path, get_error) } fn swap_data_by_column_path( @@ -192,11 +192,14 @@ pub fn get_data_by_member(value: &Value, name: &PathMember) -> Result( value: &Value, path: &ColumnPath, - callback: Box ShellError>, -) -> Result { + get_error: F, +) -> Result +where + F: FnOnce(&Value, &PathMember, ShellError) -> ShellError, +{ let mut current = value.clone(); for p in path.iter() { @@ -204,24 +207,25 @@ pub fn get_data_by_column_path( match value { Ok(v) => current = v.clone(), - Err(e) => return Err(callback((¤t, &p.clone(), e))), + Err(e) => return Err(get_error(¤t, &p, e)), } } Ok(current) } -pub fn swap_data_by_column_path( +pub fn swap_data_by_column_path( value: &Value, path: &ColumnPath, - callback: Box Result>, -) -> Result { + get_replacement: F, +) -> Result +where + F: FnOnce(&Value) -> Result, +{ let fields = path.clone(); - let to_replace = get_data_by_column_path( - &value, - path, - Box::new(move |(obj_source, column_path_tried, error)| { + let to_replace = + get_data_by_column_path(&value, path, move |obj_source, column_path_tried, error| { let path_members_span = fields.maybe_span().unwrap_or_else(Span::unknown); match &obj_source.value { @@ -304,7 +308,7 @@ pub fn swap_data_by_column_path( let primary_label = format!("There isn't a column named '{}'", &column); if let Some(suggestions) = - nu_protocol::did_you_mean(&obj_source, column_path_tried) + nu_protocol::did_you_mean(&obj_source, &column_path_tried) { return ShellError::labeled_error_with_secondary( "Unknown column", @@ -338,7 +342,7 @@ pub fn swap_data_by_column_path( _ => {} } - if let Some(suggestions) = nu_protocol::did_you_mean(&obj_source, column_path_tried) { + if let Some(suggestions) = nu_protocol::did_you_mean(&obj_source, &column_path_tried) { return ShellError::labeled_error( "Unknown column", format!("did you mean '{}'?", suggestions[0].1), @@ -347,11 +351,10 @@ pub fn swap_data_by_column_path( } error - }), - ); + }); let to_replace = to_replace?; - let replacement = callback(&to_replace)?; + let replacement = get_replacement(&to_replace)?; value .replace_data_at_column_path(&path, replacement) diff --git a/crates/nu_plugin_inc/src/inc.rs b/crates/nu_plugin_inc/src/inc.rs index 8131cb198c..4d09675223 100644 --- a/crates/nu_plugin_inc/src/inc.rs +++ b/crates/nu_plugin_inc/src/inc.rs @@ -1,7 +1,7 @@ use nu_errors::ShellError; use nu_protocol::{did_you_mean, ColumnPath, Primitive, ShellTypeName, UntaggedValue, Value}; use nu_source::{span_for_spanned_list, HasSpan, SpannedItem, Tagged}; -use nu_value_ext::ValueExt; +use nu_value_ext::{get_data_by_column_path, ValueExt}; #[derive(Debug, Eq, PartialEq)] pub enum Action { @@ -100,22 +100,24 @@ impl Inc { Some(ref f) => { let fields = f.clone(); - let replace_for = value.get_data_by_column_path( + let replace_for = get_data_by_column_path( + &value, &f, - Box::new(move |(obj_source, column_path_tried, _)| { - match did_you_mean(&obj_source, &column_path_tried) { - Some(suggestions) => ShellError::labeled_error( - "Unknown column", - format!("did you mean '{}'?", suggestions[0].1), - span_for_spanned_list(fields.iter().map(|p| p.span)), - ), - None => ShellError::labeled_error( - "Unknown column", - "row does not contain this column", - span_for_spanned_list(fields.iter().map(|p| p.span)), - ), - } - }), + move |obj_source, column_path_tried, _| match did_you_mean( + &obj_source, + &column_path_tried, + ) { + Some(suggestions) => ShellError::labeled_error( + "Unknown column", + format!("did you mean '{}'?", suggestions[0].1), + span_for_spanned_list(fields.iter().map(|p| p.span)), + ), + None => ShellError::labeled_error( + "Unknown column", + "row does not contain this column", + span_for_spanned_list(fields.iter().map(|p| p.span)), + ), + }, ); let got = replace_for?;