Merge pull request #892 from andrasio/column_path-fetch-table

Value operations and error handling separation.

src/commands/get.rs

@@ -1,8 +1,8 @@
 use crate::commands::WholeStreamCommand;
-use crate::data::meta::tag_for_tagged_list;
 use crate::data::Value;
 use crate::errors::ShellError;
 use crate::prelude::*;
+use crate::utils::did_you_mean;
 use log::trace;
 
 pub struct Get;
@@ -50,43 +50,54 @@ pub fn get_column_path(
     path: &ColumnPath,
     obj: &Tagged<Value>,
 ) -> Result<Tagged<Value>, ShellError> {
-    let mut current = Some(obj);
+    let fields = path.clone();
-    for p in path.iter() {
+
-        if let Some(obj) = current {
+    let value = obj.get_data_by_column_path(
-            current = match obj.get_data_by_key(&p) {
+        obj.tag(),
-                Some(v) => Some(v),
+        path,
-                None =>
+        Box::new(move |(obj_source, column_path_tried)| {
-                // Before we give up, see if they gave us a path that matches a field name by itself
+            match obj_source {
+                Value::Table(rows) => {
+                    let total = rows.len();
+                    let end_tag = match fields.iter().nth_back(if fields.len() > 2 { 1 } else { 0 })
                     {
-                    let possibilities = obj.data_descriptors();
+                        Some(last_field) => last_field.tag(),
+                        None => column_path_tried.tag(),
+                    };
 
-                    let mut possible_matches: Vec<_> = possibilities
+                    return ShellError::labeled_error_with_secondary(
-                        .iter()
+                        "Row not found",
-                        .map(|x| (natural::distance::levenshtein_distance(x, &p), x))
+                        format!("There isn't a row indexed at '{}'", **column_path_tried),
-                        .collect();
+                        column_path_tried.tag(),
+                        format!("The table only has {} rows (0..{})", total, total - 1),
+                        end_tag,
+                    );
+                }
+                _ => {}
+            }
 
-                    possible_matches.sort();
+            match did_you_mean(&obj_source, &column_path_tried) {
-
+                Some(suggestions) => {
-                    if possible_matches.len() > 0 {
+                    return ShellError::labeled_error(
-                        return Err(ShellError::labeled_error(
                         "Unknown column",
-                            format!("did you mean '{}'?", possible_matches[0].1),
+                        format!("did you mean '{}'?", suggestions[0].1),
-                            tag_for_tagged_list(path.iter().map(|p| p.tag())),
+                        tag_for_tagged_list(fields.iter().map(|p| p.tag())),
-                        ));
+                    )
-                    } else {
+                }
-                        return Err(ShellError::labeled_error(
+                None => {
+                    return ShellError::labeled_error(
                         "Unknown column",
                         "row does not contain this column",
-                            tag_for_tagged_list(path.iter().map(|p| p.tag())),
+                        tag_for_tagged_list(fields.iter().map(|p| p.tag())),
-                        ));
+                    )
-                    }
-                }
-            }
                 }
             }
+        }),
+    );
 
-    match current {
+    let res = match value {
-        Some(v) => Ok(v.clone()),
+        Ok(fetched) => match fetched {
+            Some(Tagged { item: v, tag }) => Ok((v.clone()).tagged(&tag)),
             None => match obj {
                 // If its None check for certain values.
                 Tagged {
@@ -99,7 +110,11 @@ pub fn get_column_path(
                 } => Ok(obj.clone()),
                 _ => Ok(Value::nothing().tagged(&obj.tag)),
             },
-    }
+        },
+        Err(reason) => Err(reason),
+    };
+
+    res
 }
 
 pub fn get(
@@ -118,26 +133,30 @@ pub fn get(
 
             let member = vec![member.clone()];
 
-            let fields = vec![&member, &fields]
+            let column_paths = vec![&member, &fields]
                 .into_iter()
                 .flatten()
                 .collect::<Vec<&ColumnPath>>();
 
-            for column_path in &fields {
+            for path in column_paths {
-                match get_column_path(column_path, &item) {
+                let res = get_column_path(&path, &item);
-                    Ok(Tagged {
+
-                        item: Value::Table(l),
+                match res {
+                    Ok(got) => match got {
+                        Tagged {
+                            item: Value::Table(rows),
                             ..
-                    }) => {
+                        } => {
-                        for item in l {
+                            for item in rows {
                                 result.push_back(ReturnSuccess::value(item.clone()));
                             }
                         }
-                    Ok(x) => result.push_back(ReturnSuccess::value(x.clone())),
+                        other => result
-                    Err(x) => result.push_back(Err(x)),
+                            .push_back(ReturnSuccess::value((*other).clone().tagged(&item.tag))),
+                    },
+                    Err(reason) => result.push_back(Err(reason)),
                 }
             }
-
             result
         })
         .flatten();

src/data/base.rs

@@ -459,11 +459,10 @@ impl Value {
         }
     }
 
-    // TODO: This is basically a legacy construct, I think
     pub fn data_descriptors(&self) -> Vec<String> {
         match self {
             Value::Primitive(_) => vec![],
-            Value::Row(o) => o
+            Value::Row(columns) => columns
                 .entries
                 .keys()
                 .into_iter()
@@ -475,6 +474,13 @@ impl Value {
         }
     }
 
+    pub(crate) fn get_data_by_index(&self, idx: usize) -> Option<&Tagged<Value>> {
+        match self {
+            Value::Table(value_set) => value_set.get(idx),
+            _ => None,
+        }
+    }
+
     pub(crate) fn get_data_by_key(&self, name: &str) -> Option<&Tagged<Value>> {
         match self {
             Value::Row(o) => o.get_data_by_key(name),
@@ -523,16 +529,22 @@ impl Value {
         &self,
         tag: Tag,
         path: &Vec<Tagged<String>>,
-    ) -> Option<Tagged<&Value>> {
+        callback: Box<dyn FnOnce((&Value, &Tagged<String>)) -> ShellError>,
+    ) -> Result<Option<Tagged<&Value>>, ShellError> {
         let mut current = self;
         for p in path {
-            match current.get_data_by_key(p) {
+            let value = match p.item().parse::<usize>() {
+                Ok(number) => current.get_data_by_index(number),
+                Err(_) => current.get_data_by_key(p),
+            };
+
+            match value {
                 Some(v) => current = v,
-                None => return None,
+                None => return Err(callback((&current.clone(), &p.clone()))),
             }
         }
 
-        Some(current.tagged(tag))
+        Ok(Some(current.tagged(tag)))
     }
 
     pub fn insert_data_at_path(
@@ -912,6 +924,7 @@ fn coerce_compare_primitive(
 mod tests {
 
     use crate::data::meta::*;
+    use crate::ShellError;
     use crate::Value;
     use indexmap::IndexMap;
 
@@ -927,6 +940,10 @@ mod tests {
         Value::table(list).tagged_unknown()
     }
 
+    fn error_callback() -> impl FnOnce((&Value, &Tagged<String>)) -> ShellError {
+        move |(_obj_source, _column_path_tried)| ShellError::unimplemented("will never be called.")
+    }
+
     fn column_path(paths: &Vec<Tagged<Value>>) -> Tagged<Vec<Tagged<String>>> {
         table(
             &paths
@@ -960,7 +977,7 @@ mod tests {
 
         let (version, tag) = string("0.4.0").into_parts();
 
-        let row = Value::row(indexmap! {
+        let value = Value::row(indexmap! {
             "package".into() =>
                 row(indexmap! {
                     "name".into()    =>     string("nu"),
@@ -969,7 +986,10 @@ mod tests {
         });
 
         assert_eq!(
-            **row.get_data_by_column_path(tag, &field_path).unwrap(),
+            **value
+                .get_data_by_column_path(tag, &field_path, Box::new(error_callback()))
+                .unwrap()
+                .unwrap(),
             version
         )
     }
@@ -980,7 +1000,7 @@ mod tests {
 
         let (name, tag) = string("Andrés N. Robalino").into_parts();
 
-        let row = Value::row(indexmap! {
+        let value = Value::row(indexmap! {
             "package".into() => row(indexmap! {
                 "name".into() => string("nu"),
                 "version".into() => string("0.4.0"),
@@ -993,11 +1013,43 @@ mod tests {
         });
 
         assert_eq!(
-            **row.get_data_by_column_path(tag, &field_path).unwrap(),
+            **value
+                .get_data_by_column_path(tag, &field_path, Box::new(error_callback()))
+                .unwrap()
+                .unwrap(),
             name
         )
     }
 
+    #[test]
+    fn column_path_that_contains_just_a_numbers_gets_a_row_from_a_table() {
+        let field_path = column_path(&vec![string("package"), string("authors"), string("0")]);
+
+        let (_, tag) = string("Andrés N. Robalino").into_parts();
+
+        let value = Value::row(indexmap! {
+            "package".into() => row(indexmap! {
+                "name".into() => string("nu"),
+                "version".into() => string("0.4.0"),
+                "authors".into() => table(&vec![
+                    row(indexmap!{"name".into() => string("Andrés N. Robalino")}),
+                    row(indexmap!{"name".into() => string("Jonathan Turner")}),
+                    row(indexmap!{"name".into() => string("Yehuda Katz")})
+                ])
+            })
+        });
+
+        assert_eq!(
+            **value
+                .get_data_by_column_path(tag, &field_path, Box::new(error_callback()))
+                .unwrap()
+                .unwrap(),
+            Value::row(indexmap! {
+                "name".into() => string("Andrés N. Robalino")
+            })
+        );
+    }
+
     #[test]
     fn replaces_matching_field_from_a_row() {
         let field_path = column_path(&vec![string("amigos")]);

src/lib.rs

@@ -30,12 +30,12 @@ pub use crate::env::host::BasicHost;
 pub use crate::parser::hir::SyntaxShape;
 pub use crate::parser::parse::token_tree_builder::TokenTreeBuilder;
 pub use crate::plugin::{serve_plugin, Plugin};
-pub use crate::utils::{AbsoluteFile, AbsolutePath, RelativePath};
+pub use crate::utils::{did_you_mean, AbsoluteFile, AbsolutePath, RelativePath};
 pub use cli::cli;
 pub use data::base::{Primitive, Value};
 pub use data::config::{config_path, APP_INFO};
 pub use data::dict::{Dictionary, TaggedDictBuilder};
-pub use data::meta::{Span, Spanned, SpannedItem, Tag, Tagged, TaggedItem};
+pub use data::meta::{tag_for_tagged_list, Span, Spanned, SpannedItem, Tag, Tagged, TaggedItem};
 pub use errors::{CoerceInto, ShellError};
 pub use num_traits::cast::ToPrimitive;
 pub use parser::parse::text::Text;

src/plugins/inc.rs

@@ -1,6 +1,6 @@
 use nu::{
-    serve_plugin, CallInfo, Plugin, Primitive, ReturnSuccess, ReturnValue, ShellError, Signature,
+    did_you_mean, serve_plugin, tag_for_tagged_list, CallInfo, Plugin, Primitive, ReturnSuccess,
-    SyntaxShape, Tagged, TaggedItem, Value,
+    ReturnValue, ShellError, Signature, SyntaxShape, Tagged, TaggedItem, Value,
 };
 
 enum Action {
@@ -93,9 +93,36 @@ impl Inc {
                     ));
                 }
             }
+
             Value::Row(_) => match self.field {
                 Some(ref f) => {
-                    let replacement = match value.item.get_data_by_column_path(value.tag(), f) {
+                    let fields = f.clone();
+
+                    let replace_for = value.item.get_data_by_column_path(
+                        value.tag(),
+                        &f,
+                        Box::new(move |(obj_source, column_path_tried)| {
+                            match did_you_mean(&obj_source, &column_path_tried) {
+                                Some(suggestions) => {
+                                    return ShellError::labeled_error(
+                                        "Unknown column",
+                                        format!("did you mean '{}'?", suggestions[0].1),
+                                        tag_for_tagged_list(fields.iter().map(|p| p.tag())),
+                                    )
+                                }
+                                None => {
+                                    return ShellError::labeled_error(
+                                        "Unknown column",
+                                        "row does not contain this column",
+                                        tag_for_tagged_list(fields.iter().map(|p| p.tag())),
+                                    )
+                                }
+                            }
+                        }),
+                    );
+
+                    let replacement = match replace_for {
+                        Ok(got) => match got {
                             Some(result) => self.inc(result.map(|x| x.clone()))?,
                             None => {
                                 return Err(ShellError::labeled_error(
@@ -104,11 +131,13 @@ impl Inc {
                                     value.tag(),
                                 ))
                             }
+                        },
+                        Err(reason) => return Err(reason),
                     };
 
                     match value.item.replace_data_at_column_path(
                         value.tag(),
-                        f,
+                        &f,
                         replacement.item.clone(),
                     ) {
                         Some(v) => return Ok(v),

src/plugins/str.rs

@@ -1,6 +1,6 @@
 use nu::{
-    serve_plugin, CallInfo, Plugin, Primitive, ReturnSuccess, ReturnValue, ShellError, Signature,
+    did_you_mean, serve_plugin, tag_for_tagged_list, CallInfo, Plugin, Primitive, ReturnSuccess,
-    SyntaxShape, Tagged, TaggedItem, Value,
+    ReturnValue, ShellError, Signature, SyntaxShape, Tagged, TaggedItem, Value,
 };
 
 #[derive(Debug, Eq, PartialEq)]
@@ -92,13 +92,48 @@ impl Str {
             Value::Primitive(Primitive::String(ref s)) => Ok(self.apply(&s)?.tagged(value.tag())),
             Value::Row(_) => match self.field {
                 Some(ref f) => {
-                    let replacement = match value.item.get_data_by_column_path(value.tag(), f) {
+                    let fields = f.clone();
+
+                    let replace_for = value.item.get_data_by_column_path(
+                        value.tag(),
+                        &f,
+                        Box::new(move |(obj_source, column_path_tried)| {
+                            match did_you_mean(&obj_source, &column_path_tried) {
+                                Some(suggestions) => {
+                                    return ShellError::labeled_error(
+                                        "Unknown column",
+                                        format!("did you mean '{}'?", suggestions[0].1),
+                                        tag_for_tagged_list(fields.iter().map(|p| p.tag())),
+                                    )
+                                }
+                                None => {
+                                    return ShellError::labeled_error(
+                                        "Unknown column",
+                                        "row does not contain this column",
+                                        tag_for_tagged_list(fields.iter().map(|p| p.tag())),
+                                    )
+                                }
+                            }
+                        }),
+                    );
+
+                    let replacement = match replace_for {
+                        Ok(got) => match got {
                             Some(result) => self.strutils(result.map(|x| x.clone()))?,
-                        None => return Ok(Value::nothing().tagged(value.tag)),
+                            None => {
+                                return Err(ShellError::labeled_error(
+                                    "inc could not find field to replace",
+                                    "column name",
+                                    value.tag(),
+                                ))
+                            }
+                        },
+                        Err(reason) => return Err(reason),
                     };
+
                     match value.item.replace_data_at_column_path(
                         value.tag(),
-                        f,
+                        &f,
                         replacement.item.clone(),
                     ) {
                         Some(v) => return Ok(v),

src/prelude.rs

@@ -66,7 +66,9 @@ pub(crate) use crate::commands::RawCommandArgs;
 pub(crate) use crate::context::CommandRegistry;
 pub(crate) use crate::context::{AnchorLocation, Context};
 pub(crate) use crate::data::base as value;
-pub(crate) use crate::data::meta::{Span, Spanned, SpannedItem, Tag, Tagged, TaggedItem};
+pub(crate) use crate::data::meta::{
+    tag_for_tagged_list, Span, Spanned, SpannedItem, Tag, Tagged, TaggedItem,
+};
 pub(crate) use crate::data::types::ExtractType;
 pub(crate) use crate::data::{Primitive, Value};
 pub(crate) use crate::env::host::handle_unexpected;

src/utils.rs

@@ -5,6 +5,30 @@ use std::fmt;
 use std::ops::Div;
 use std::path::{Component, Path, PathBuf};
 
+pub fn did_you_mean(
+    obj_source: &Value,
+    field_tried: &Tagged<String>,
+) -> Option<Vec<(usize, String)>> {
+    let possibilities = obj_source.data_descriptors();
+
+    let mut possible_matches: Vec<_> = possibilities
+        .into_iter()
+        .map(|x| {
+            let word = x.clone();
+            let distance = natural::distance::levenshtein_distance(&word, &field_tried);
+
+            (distance, word)
+        })
+        .collect();
+
+    if possible_matches.len() > 0 {
+        possible_matches.sort();
+        return Some(possible_matches);
+    }
+
+    None
+}
+
 pub struct AbsoluteFile {
     inner: PathBuf,
 }

tests/command_get_tests.rs

@@ -0,0 +1,126 @@
+mod helpers;
+
+use helpers as h;
+use helpers::{Playground, Stub::*};
+
+#[test]
+fn get() {
+    Playground::setup("get_test_1", |dirs, sandbox| {
+        sandbox.with_files(vec![FileWithContent(
+            "sample.toml",
+            r#"
+                nu_party_venue = "zion"
+            "#,
+        )]);
+
+        let actual = nu!(
+            cwd: dirs.test(), h::pipeline(
+            r#"
+                open sample.toml
+                | get nu_party_venue
+                | echo $it
+            "#
+        ));
+
+        assert_eq!(actual, "zion");
+    })
+}
+
+#[test]
+fn fetches_by_index_from_a_given_table() {
+    Playground::setup("get_test_2", |dirs, sandbox| {
+        sandbox.with_files(vec![FileWithContent(
+            "sample.toml",
+            r#"
+                [package]
+                name = "nu"
+                version = "0.4.1"
+                authors = ["Yehuda Katz <wycats@gmail.com>", "Jonathan Turner <jonathan.d.turner@gmail.com>", "Andrés N. Robalino <andres@androbtech.com>"]
+                description = "When arepas shells are tasty and fun."
+            "#,
+        )]);
+
+        let actual = nu!(
+            cwd: dirs.test(), h::pipeline(
+            r#"
+                open sample.toml
+                | get package.authors.2
+                | echo $it
+            "#
+        ));
+
+        assert_eq!(actual, "Andrés N. Robalino <andres@androbtech.com>");
+    })
+}
+
+#[test]
+fn fetches_more_than_one_column_member_path() {
+    Playground::setup("get_test_3", |dirs, sandbox| {
+        sandbox.with_files(vec![FileWithContent(
+            "sample.toml",
+            r#"
+                [[fortune_tellers]]
+                name = "Andrés N. Robalino"
+                arepas = 1
+
+                [[fortune_tellers]]
+                name = "Jonathan Turner"
+                arepas = 1
+
+                [[fortune_tellers]]
+                name = "Yehuda Katz"
+                arepas = 1
+            "#,
+        )]);
+
+        let actual = nu!(
+            cwd: dirs.test(), h::pipeline(
+            r#"
+                open sample.toml
+                | get fortune_tellers.2.name fortune_tellers.0.name fortune_tellers.1.name
+                | nth 2
+                | echo $it
+            "#
+        ));
+
+        assert_eq!(actual, "Jonathan Turner");
+    })
+}
+
+#[test]
+fn errors_fetching_by_index_out_of_bounds_from_table() {
+    Playground::setup("get_test_4", |dirs, sandbox| {
+        sandbox.with_files(vec![FileWithContent(
+            "sample.toml",
+            r#"
+                [spanish_lesson]
+                sentence_words = ["Yo", "quiero", "taconushell"]
+            "#,
+        )]);
+
+        let actual = nu_error!(
+            cwd: dirs.test(), h::pipeline(
+            r#"
+                open sample.toml
+                | get spanish_lesson.sentence_words.3
+            "#
+        ));
+
+        assert!(actual.contains("Row not found"));
+        assert!(actual.contains("There isn't a row indexed at '3'"));
+        assert!(actual.contains("The table only has 3 rows (0..2)"))
+    })
+}
+
+#[test]
+fn requires_at_least_one_column_member_path() {
+    Playground::setup("get_test_5", |dirs, sandbox| {
+        sandbox.with_files(vec![EmptyFile("andres.txt")]);
+
+        let actual = nu_error!(
+            cwd: dirs.test(), "ls | get"
+        );
+
+        assert!(actual.contains("requires member parameter"));
+    })
+}