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

@@ -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,
+    did_you_mean, ColumnPath, Dictionary, PathMember, Primitive, ReturnSuccess, Signature,
-    UnspannedPathMember, UntaggedValue, Value,
+    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<Value, ShellError> {
-    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<String> = 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::<Vec<String>>()
-                                        .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<OutputStream, ShellError> {
     let registry = registry.clone();
-    let (GetArgs { rest: mut fields }, mut input) = args.process(&registry).await?;
+    let (GetArgs { rest: column_paths }, mut input) = args.process(&registry).await?;
-    if fields.is_empty() {
+    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 {:?}", column_paths);
-        trace!("get {:?} {:?}", member, fields);
+        let output_stream = input
-
-        Ok(input
             .map(move |item| {
-                let member = vec![member.clone()];
+                let output = column_paths
-
+                    .iter()
-                let column_paths = vec![&member, &fields]
+                    .map(move |path| get_output(&item, path))
-                    .into_iter()
                     .flatten()
-                    .collect::<Vec<&ColumnPath>>();
+                    .collect::<Vec<_>>();
-
-                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(),
-                        )),
-                    }
-                }
-
                 futures::stream::iter(output)
             })
             .flatten()
-            .to_output_stream())
+            .to_output_stream();
+        Ok(output_stream)
+    }
+}
+
+fn get_output(item: &Value, path: &ColumnPath) -> Vec<Result<ReturnSuccess, ShellError>> {
+    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<Value, ShellError> {
+    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<String> = 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::<Vec<String>>()
+                            .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<ShellError> {
+    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),
+        )),
     }
 }
 

crates/nu-cli/src/commands/histogram.rs

@@ -178,11 +178,7 @@ fn evaluator(by: ColumnPath) -> Box<dyn Fn(usize, &Value) -> Result<Value, Shell
     Box::new(move |_: usize, value: &Value| {
         let path = by.clone();
 
-        let eval = nu_value_ext::get_data_by_column_path(
+        let eval = nu_value_ext::get_data_by_column_path(value, &path, move |_, _, error| error);
-            value,
-            &path,
-            Box::new(move |(_, _, error)| error),
-        );
 
         match eval {
             Ok(with_value) => Ok(with_value),

crates/nu-cli/src/commands/select.rs

@@ -83,7 +83,7 @@ async fn select(args: CommandArgs, registry: &CommandRegistry) -> Result<OutputS
             let fetcher = get_data_by_column_path(
                 &value,
                 &path,
-                Box::new(move |(obj_source, path_member_tried, error)| {
+                move |obj_source, path_member_tried, error| {
                     if let PathMember {
                         unspanned: UnspannedPathMember::String(column),
                         ..
@@ -98,7 +98,7 @@ async fn select(args: CommandArgs, registry: &CommandRegistry) -> Result<OutputS
                     }
 
                     error
-                }),
+                },
             );
 
             let field = path.clone();

crates/nu-data/src/base.rs

@@ -190,8 +190,8 @@ mod tests {
 
     fn error_callback(
         reason: &'static str,
-    ) -> impl FnOnce((&Value, &PathMember, ShellError)) -> ShellError {
+    ) -> impl FnOnce(&Value, &PathMember, ShellError) -> ShellError {
-        move |(_obj_source, _column_path_tried, _err)| ShellError::unimplemented(reason)
+        move |_obj_source, _column_path_tried, _err| ShellError::unimplemented(reason)
     }
 
     fn column_path(paths: &[Value]) -> Result<Tagged<ColumnPathValue>, ShellError> {

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<dyn FnOnce((&Value, &PathMember, ShellError)) -> ShellError>,
+        callback: Box<dyn FnOnce(&Value, &PathMember, ShellError) -> ShellError>,
     ) -> Result<Value, ShellError>;
     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<dyn FnOnce((&Value, &PathMember, ShellError)) -> ShellError>,
+        get_error: Box<dyn FnOnce(&Value, &PathMember, ShellError) -> ShellError>,
     ) -> Result<Value, ShellError> {
-        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, She
     }
 }
 
-pub fn get_data_by_column_path(
+pub fn get_data_by_column_path<F>(
     value: &Value,
     path: &ColumnPath,
-    callback: Box<dyn FnOnce((&Value, &PathMember, ShellError)) -> ShellError>,
+    get_error: F,
-) -> Result<Value, ShellError> {
+) -> Result<Value, ShellError>
+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((&current, &p.clone(), e))),
+            Err(e) => return Err(get_error(&current, &p, e)),
         }
     }
 
     Ok(current)
 }
 
-pub fn swap_data_by_column_path(
+pub fn swap_data_by_column_path<F>(
     value: &Value,
     path: &ColumnPath,
-    callback: Box<dyn FnOnce(&Value) -> Result<Value, ShellError>>,
+    get_replacement: F,
-) -> Result<Value, ShellError> {
+) -> Result<Value, ShellError>
+where
+    F: FnOnce(&Value) -> Result<Value, ShellError>,
+{
     let fields = path.clone();
 
-    let to_replace = get_data_by_column_path(
+    let to_replace =
-        &value,
+        get_data_by_column_path(&value, path, move |obj_source, column_path_tried, error| {
-        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 {
@@ -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)

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, _)| {
+                        move |obj_source, column_path_tried, _| match did_you_mean(
-                            match did_you_mean(&obj_source, &column_path_tried) {
+                            &obj_source,
-                                Some(suggestions) => ShellError::labeled_error(
+                            &column_path_tried,
-                                    "Unknown column",
+                        ) {
-                                    format!("did you mean '{}'?", suggestions[0].1),
+                            Some(suggestions) => ShellError::labeled_error(
-                                    span_for_spanned_list(fields.iter().map(|p| p.span)),
+                                "Unknown column",
-                                ),
+                                format!("did you mean '{}'?", suggestions[0].1),
-                                None => ShellError::labeled_error(
+                                span_for_spanned_list(fields.iter().map(|p| p.span)),
-                                    "Unknown column",
+                            ),
-                                    "row does not contain this column",
+                            None => ShellError::labeled_error(
-                                    span_for_spanned_list(fields.iter().map(|p| p.span)),
+                                "Unknown column",
-                                ),
+                                "row does not contain this column",
-                            }
+                                span_for_spanned_list(fields.iter().map(|p| p.span)),
-                        }),
+                            ),
+                        },
                     );
 
                     let got = replace_for?;