Merge pull request #914 from andrasio/column_path-semantic-fix

ColumnPaths should expect members encapsulated as members.

src/commands/get.rs

@@ -44,7 +44,7 @@ impl WholeStreamCommand for Get {
     }
 }
 
-pub type ColumnPath = Vec<Tagged<String>>;
+pub type ColumnPath = Vec<Tagged<Value>>;
 
 pub fn get_column_path(
     path: &ColumnPath,
@@ -67,7 +67,13 @@ pub fn get_column_path(
 
                     return ShellError::labeled_error_with_secondary(
                         "Row not found",
-                        format!("There isn't a row indexed at '{}'", **column_path_tried),
+                        format!(
+                            "There isn't a row indexed at '{}'",
+                            match &*column_path_tried {
+                                Value::Primitive(primitive) => primitive.format(None),
+                                _ => String::from(""),
+                            }
+                        ),
                         column_path_tried.tag(),
                         format!("The table only has {} rows (0..{})", total, total - 1),
                         end_tag,
@@ -76,7 +82,21 @@ pub fn get_column_path(
                 _ => {}
             }
 
-            match did_you_mean(&obj_source, &column_path_tried) {
+            match &column_path_tried {
+                Tagged {
+                    item: Value::Primitive(Primitive::Int(index)),
+                    ..
+                } => {
+                    return ShellError::labeled_error(
+                        "No rows available",
+                        format!(
+                            "Not a table. Perhaps you meant to get the column '{}' instead?",
+                            index
+                        ),
+                        column_path_tried.tag(),
+                    )
+                }
+                _ => match did_you_mean(&obj_source, &column_path_tried) {
                     Some(suggestions) => {
                         return ShellError::labeled_error(
                             "Unknown column",
@@ -91,6 +111,7 @@ pub fn get_column_path(
                             tag_for_tagged_list(fields.iter().map(|p| p.tag())),
                         )
                     }
+                },
             }
         }),
     );

src/data/base.rs

@@ -409,27 +409,19 @@ impl Tagged<Value> {
         ValueDebug { value: self }
     }
 
-    pub fn as_column_path(&self) -> Result<Tagged<Vec<Tagged<String>>>, ShellError> {
+    pub fn as_column_path(&self) -> Result<Tagged<Vec<Tagged<Value>>>, ShellError> {
-        let mut out: Vec<Tagged<String>> = vec![];
-
         match &self.item {
-            Value::Table(table) => {
+            Value::Primitive(Primitive::String(s)) => {
-                for item in table {
+                Ok(vec![Value::string(s).tagged(&self.tag)].tagged(&self.tag))
-                    out.push(item.as_string()?.tagged(&item.tag));
             }
-            }
+            Value::Table(table) => Ok(table.to_vec().tagged(&self.tag)),
-
+            other => Err(ShellError::type_error(
-            other => {
-                return Err(ShellError::type_error(
                 "column name",
                 other.type_name().tagged(&self.tag),
-                ))
+            )),
         }
     }
 
-        Ok(out.tagged(&self.tag))
-    }
-
     pub(crate) fn as_string(&self) -> Result<String, ShellError> {
         match &self.item {
             Value::Primitive(Primitive::String(s)) => Ok(s.clone()),
@@ -528,95 +520,62 @@ impl Value {
     pub fn get_data_by_column_path(
         &self,
         tag: Tag,
-        path: &Vec<Tagged<String>>,
+        path: &Vec<Tagged<Value>>,
-        callback: Box<dyn FnOnce((&Value, &Tagged<String>)) -> ShellError>,
+        callback: Box<dyn FnOnce((Value, Tagged<Value>)) -> ShellError>,
     ) -> Result<Option<Tagged<&Value>>, ShellError> {
+        let mut column_path = vec![];
+
+        for value in path {
+            column_path.push(
+                Value::string(value.as_string().unwrap_or("".to_string())).tagged(&value.tag),
+            );
+        }
+
+        let path = column_path;
+
         let mut current = self;
+
         for p in path {
-            // note:
+            let value = p.as_string().unwrap_or("".to_string());
-            // This will eventually be refactored once we are able
+            let value = match value.parse::<usize>() {
-            // to parse correctly column_paths and get them deserialized
-            // to values for us.
-            let value = match p.item().parse::<usize>() {
                 Ok(number) => match current {
                     Value::Table(_) => current.get_data_by_index(number),
-                    Value::Row(_) => current.get_data_by_key(p),
+                    Value::Row(_) => current.get_data_by_key(&value),
                     _ => None,
                 },
                 Err(_) => match self {
-                    Value::Table(_) | Value::Row(_) => current.get_data_by_key(p),
+                    Value::Table(_) | Value::Row(_) => current.get_data_by_key(&value),
                     _ => None,
                 },
-            }; // end
+            };
 
             match value {
                 Some(v) => current = v,
-                None => return Err(callback((&current.clone(), &p.clone()))),
+                None => return Err(callback((current.clone(), p.clone()))),
             }
         }
 
         Ok(Some(current.tagged(tag)))
     }
 
-    pub fn insert_data_at_path(
-        &self,
-        tag: Tag,
-        path: &str,
-        new_value: Value,
-    ) -> Option<Tagged<Value>> {
-        let mut new_obj = self.clone();
-
-        let split_path: Vec<_> = path.split(".").collect();
-
-        if let Value::Row(ref mut o) = new_obj {
-            let mut current = o;
-
-            if split_path.len() == 1 {
-                // Special case for inserting at the top level
-                current
-                    .entries
-                    .insert(path.to_string(), new_value.tagged(&tag));
-                return Some(new_obj.tagged(&tag));
-            }
-
-            for idx in 0..split_path.len() {
-                match current.entries.get_mut(split_path[idx]) {
-                    Some(next) => {
-                        if idx == (split_path.len() - 2) {
-                            match &mut next.item {
-                                Value::Row(o) => {
-                                    o.entries.insert(
-                                        split_path[idx + 1].to_string(),
-                                        new_value.tagged(&tag),
-                                    );
-                                }
-                                _ => {}
-                            }
-
-                            return Some(new_obj.tagged(&tag));
-                        } else {
-                            match next.item {
-                                Value::Row(ref mut o) => {
-                                    current = o;
-                                }
-                                _ => return None,
-                            }
-                        }
-                    }
-                    _ => return None,
-                }
-            }
-        }
-
-        None
-    }
-
     pub fn insert_data_at_column_path(
         &self,
         tag: Tag,
-        split_path: &Vec<Tagged<String>>,
+        split_path: &Vec<Tagged<Value>>,
         new_value: Value,
     ) -> Option<Tagged<Value>> {
+        let split_path = split_path
+            .into_iter()
+            .map(|p| match p {
+                Tagged {
+                    item: Value::Primitive(Primitive::String(s)),
+                    tag,
+                } => Ok(s.clone().tagged(tag)),
+                o => Err(o),
+            })
+            .filter_map(Result::ok)
+            .collect::<Vec<Tagged<String>>>();
+
         let mut new_obj = self.clone();
 
         if let Value::Row(ref mut o) = new_obj {
@@ -665,9 +624,21 @@ impl Value {
     pub fn replace_data_at_column_path(
         &self,
         tag: Tag,
-        split_path: &Vec<Tagged<String>>,
+        split_path: &Vec<Tagged<Value>>,
         replaced_value: Value,
     ) -> Option<Tagged<Value>> {
+        let split_path = split_path
+            .into_iter()
+            .map(|p| match p {
+                Tagged {
+                    item: Value::Primitive(Primitive::String(s)),
+                    tag,
+                } => Ok(s.clone().tagged(tag)),
+                o => Err(o),
+            })
+            .filter_map(Result::ok)
+            .collect::<Vec<Tagged<String>>>();
+
         let mut new_obj = self.clone();
         let mut current = &mut new_obj;
 
@@ -943,6 +914,10 @@ mod tests {
         Value::string(input.into()).tagged_unknown()
     }
 
+    fn number(n: i64) -> Tagged<Value> {
+        Value::number(n).tagged_unknown()
+    }
+
     fn row(entries: IndexMap<String, Tagged<Value>>) -> Tagged<Value> {
         Value::row(entries).tagged_unknown()
     }
@@ -951,19 +926,12 @@ mod tests {
         Value::table(list).tagged_unknown()
     }
 
-    fn error_callback() -> impl FnOnce((&Value, &Tagged<String>)) -> ShellError {
+    fn error_callback() -> impl FnOnce((Value, Tagged<Value>)) -> ShellError {
         move |(_obj_source, _column_path_tried)| ShellError::unimplemented("will never be called.")
     }
 
-    fn column_path(paths: &Vec<Tagged<Value>>) -> Tagged<Vec<Tagged<String>>> {
+    fn column_path(paths: &Vec<Tagged<Value>>) -> Vec<Tagged<Value>> {
-        table(
+        table(paths).as_column_path().unwrap().item
-            &paths
-                .iter()
-                .map(|p| string(p.as_string().unwrap()))
-                .collect(),
-        )
-        .as_column_path()
-        .unwrap()
     }
 
     #[test]
@@ -1005,36 +973,9 @@ mod tests {
         )
     }
 
-    #[test]
-    fn gets_first_matching_field_from_rows_with_same_field_inside_a_table() {
-        let field_path = column_path(&vec![string("package"), string("authors"), string("name")]);
-
-        let (name, 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(),
-            name
-        )
-    }
-
     #[test]
     fn column_path_that_contains_just_a_number_gets_a_row_from_a_table() {
-        let field_path = column_path(&vec![string("package"), string("authors"), string("0")]);
+        let field_path = column_path(&vec![string("package"), string("authors"), number(0)]);
 
         let (_, tag) = string("Andrés N. Robalino").into_parts();
 

src/parser/deserializer.rs

@@ -61,7 +61,7 @@ impl<'de> ConfigDeserializer<'de> {
     pub fn top(&mut self) -> &DeserializerItem {
         let value = self.stack.last();
         trace!("inspecting top value :: {:?}", value);
-        value.expect("Can't get top elemant of an empty stack")
+        value.expect("Can't get top element of an empty stack")
     }
 
     pub fn pop(&mut self) -> DeserializerItem {
@@ -486,8 +486,8 @@ mod tests {
         // is unspecified and change is likely.
         // This test makes sure that such change is detected
         // by this test failing, and not things silently breaking.
-        // Specifically, we rely on this behaviour further above
+        // Specifically, we rely on this behavior further above
-        // in the file to special case Tagged<Value> parsing.
+        // in the file for the Tagged<Value> special case parsing.
         let tuple = type_name::<()>();
         let tagged_tuple = type_name::<Tagged<()>>();
         let tagged_value = type_name::<Tagged<Value>>();

src/parser/hir/syntax_shape/expression/number.rs

@@ -1,7 +1,8 @@
 use crate::parser::hir::syntax_shape::{
     expand_atom, parse_single_node, ExpandContext, ExpandExpression, ExpansionRule,
-    FallibleColorSyntax, FlatShape, ParseError,
+    FallibleColorSyntax, FlatShape, ParseError, TestSyntax,
 };
+use crate::parser::hir::tokens_iterator::Peeked;
 use crate::parser::{
     hir,
     hir::{RawNumber, TokensIterator},
@@ -212,3 +213,18 @@ impl FallibleColorSyntax for IntShape {
         Ok(())
     }
 }
+
+impl TestSyntax for NumberShape {
+    fn test<'a, 'b>(
+        &self,
+        token_nodes: &'b mut TokensIterator<'a>,
+        _context: &ExpandContext,
+    ) -> Option<Peeked<'a, 'b>> {
+        let peeked = token_nodes.peek_any();
+
+        match peeked.node {
+            Some(token) if token.is_number() => Some(peeked),
+            _ => None,
+        }
+    }
+}

src/parser/hir/syntax_shape/expression/variable_path.rs

@@ -906,6 +906,16 @@ impl ExpandSyntax for MemberShape {
             return Ok(Member::Bare(node.span()));
         }
 
+        /* KATZ */
+        /* let number = NumberShape.test(token_nodes, context);
+
+        if let Some(peeked) = number {
+            let node = peeked.not_eof("column")?.commit();
+            let (n, span) = node.as_number().unwrap();
+
+            return Ok(Member::Number(n, span))
+        }*/
+
         let string = StringShape.test(token_nodes, context);
 
         if let Some(peeked) = string {

src/parser/hir/tokens_iterator/tests.rs

@@ -0,0 +1,16 @@
+use crate::parser::hir::TokensIterator;
+use crate::parser::parse::token_tree_builder::TokenTreeBuilder as b;
+use crate::Span;
+
+#[test]
+fn supplies_tokens() {
+    let tokens = b::token_list(vec![b::var("it"), b::op("."), b::bare("cpu")]);
+    let (tokens, _) = b::build(tokens);
+
+    let tokens = tokens.expect_list();
+    let mut iterator = TokensIterator::all(tokens, Span::unknown());
+
+    iterator.next().unwrap().expect_var();
+    iterator.next().unwrap().expect_dot();
+    iterator.next().unwrap().expect_bare();
+}

src/parser/parse/token_tree.rs

@@ -171,6 +171,16 @@ impl TokenNode {
         }
     }
 
+    pub fn is_number(&self) -> bool {
+        match self {
+            TokenNode::Token(Spanned {
+                item: RawToken::Number(_),
+                ..
+            }) => true,
+            _ => false,
+        }
+    }
+
     pub fn as_string(&self) -> Option<(Span, Span)> {
         match self {
             TokenNode::Token(Spanned {

src/plugins/edit.rs

@@ -3,7 +3,7 @@ use nu::{
     Tagged, Value,
 };
 
-pub type ColumnPath = Tagged<Vec<Tagged<String>>>;
+pub type ColumnPath = Tagged<Vec<Tagged<Value>>>;
 
 struct Edit {
     field: Option<ColumnPath>,

src/plugins/inc.rs

@@ -14,7 +14,7 @@ pub enum SemVerAction {
     Patch,
 }
 
-pub type ColumnPath = Vec<Tagged<String>>;
+pub type ColumnPath = Tagged<Vec<Tagged<Value>>>;
 
 struct Inc {
     field: Option<ColumnPath>,
@@ -100,7 +100,7 @@ impl Inc {
 
                     let replace_for = value.item.get_data_by_column_path(
                         value.tag(),
-                        &f,
+                        f,
                         Box::new(move |(obj_source, column_path_tried)| {
                             match did_you_mean(&obj_source, &column_path_tried) {
                                 Some(suggestions) => {
@@ -191,7 +191,7 @@ impl Plugin for Inc {
                         item: Value::Table(_),
                         ..
                     } => {
-                        self.field = Some(table.as_column_path()?.item().to_vec());
+                        self.field = Some(table.as_column_path()?);
                     }
                     value => return Err(ShellError::type_error("table", value.tagged_type_name())),
                 }
@@ -229,8 +229,8 @@ mod tests {
     use super::{Inc, SemVerAction};
     use indexmap::IndexMap;
     use nu::{
-        CallInfo, EvaluatedArgs, Plugin, ReturnSuccess, Tag, Tagged, TaggedDictBuilder, TaggedItem,
+        CallInfo, EvaluatedArgs, Plugin, Primitive, ReturnSuccess, Tag, Tagged, TaggedDictBuilder,
-        Value,
+        TaggedItem, Value,
     };
 
     struct CallStub {
@@ -344,9 +344,13 @@ mod tests {
             .is_ok());
 
         assert_eq!(
-            plugin
+            plugin.field.map(|f| f
-                .field
+                .iter()
-                .map(|f| f.iter().map(|f| f.item.clone()).collect()),
+                .map(|f| match &f.item {
+                    Value::Primitive(Primitive::String(s)) => s.clone(),
+                    _ => panic!(""),
+                })
+                .collect()),
             Some(vec!["package".to_string(), "version".to_string()])
         );
     }

src/plugins/insert.rs

@@ -4,7 +4,7 @@ use nu::{
     Tagged, TaggedItem, Value,
 };
 
-pub type ColumnPath = Vec<Tagged<String>>;
+pub type ColumnPath = Vec<Tagged<Value>>;
 
 struct Insert {
     field: Option<ColumnPath>,
@@ -22,14 +22,21 @@ impl Insert {
         let value_tag = value.tag();
         match (value.item, self.value.clone()) {
             (obj @ Value::Row(_), Some(v)) => match &self.field {
-                Some(f) => match obj.insert_data_at_column_path(value_tag.clone(), &f, v) {
+                Some(f) => match obj.insert_data_at_column_path(value_tag.clone(), f, v) {
                     Some(v) => return Ok(v),
                     None => {
                         return Err(ShellError::labeled_error(
                             format!(
                                 "add could not find place to insert field {:?} {}",
                                 obj,
-                                f.iter().map(|i| &i.item).join(".")
+                                f.iter()
+                                    .map(|i| {
+                                        match &i.item {
+                                            Value::Primitive(primitive) => primitive.format(None),
+                                            _ => String::from(""),
+                                        }
+                                    })
+                                    .join(".")
                             ),
                             "column name",
                             &value_tag,

src/plugins/str.rs

@@ -12,7 +12,7 @@ enum Action {
     Substring(usize, usize),
 }
 
-pub type ColumnPath = Vec<Tagged<String>>;
+pub type ColumnPath = Tagged<Vec<Tagged<Value>>>;
 
 struct Str {
     field: Option<ColumnPath>,
@@ -132,7 +132,7 @@ impl Str {
 
                     let replace_for = value.item.get_data_by_column_path(
                         value.tag(),
-                        &f,
+                        f,
                         Box::new(move |(obj_source, column_path_tried)| {
                             match did_you_mean(&obj_source, &column_path_tried) {
                                 Some(suggestions) => {
@@ -169,7 +169,7 @@ impl Str {
 
                     match value.item.replace_data_at_column_path(
                         value.tag(),
-                        &f,
+                        f,
                         replacement.item.clone(),
                     ) {
                         Some(v) => return Ok(v),
@@ -246,17 +246,17 @@ impl Plugin for Str {
 
         if let Some(possible_field) = args.nth(0) {
             match possible_field {
-                Tagged {
+                string @ Tagged {
-                    item: Value::Primitive(Primitive::String(s)),
+                    item: Value::Primitive(Primitive::String(_)),
-                    tag,
+                    ..
                 } => {
-                    self.for_field(vec![s.clone().tagged(tag)]);
+                    self.for_field(string.as_column_path()?);
                 }
                 table @ Tagged {
                     item: Value::Table(_),
                     ..
                 } => {
-                    self.field = Some(table.as_column_path()?.item);
+                    self.field = Some(table.as_column_path()?);
                 }
                 _ => {
                     return Err(ShellError::labeled_error(
@@ -419,9 +419,13 @@ mod tests {
             .is_ok());
 
         assert_eq!(
-            plugin
+            plugin.field.map(|f| f
-                .field
+                .iter()
-                .map(|f| f.into_iter().map(|f| f.item).collect()),
+                .map(|f| match &f.item {
+                    Value::Primitive(Primitive::String(s)) => s.clone(),
+                    _ => panic!(""),
+                })
+                .collect()),
             Some(vec!["package".to_string(), "description".to_string()])
         )
     }

src/utils.rs

@@ -7,8 +7,10 @@ use std::path::{Component, Path, PathBuf};
 
 pub fn did_you_mean(
     obj_source: &Value,
-    field_tried: &Tagged<String>,
+    field_tried: &Tagged<Value>,
 ) -> Option<Vec<(usize, String)>> {
+    let field_tried = field_tried.as_string().unwrap();
+
     let possibilities = obj_source.data_descriptors();
 
     let mut possible_matches: Vec<_> = possibilities

tests/command_get_tests.rs

@@ -27,7 +27,7 @@ fn get() {
 }
 
 #[test]
-fn fetches_by_index_from_a_given_table() {
+fn fetches_by_index() {
     Playground::setup("get_test_2", |dirs, sandbox| {
         sandbox.with_files(vec![FileWithContent(
             "sample.toml",
@@ -53,14 +53,13 @@ fn fetches_by_index_from_a_given_table() {
     })
 }
 #[test]
-fn supports_fetching_rows_from_tables_using_columns_named_as_numbers() {
+fn fetches_by_column_path() {
     Playground::setup("get_test_3", |dirs, sandbox| {
         sandbox.with_files(vec![FileWithContent(
             "sample.toml",
             r#"
                 [package]
-                0 = "nu"
+                name = "nu"
-                1 = "0.4.1"
             "#,
         )]);
 
@@ -68,25 +67,23 @@ fn supports_fetching_rows_from_tables_using_columns_named_as_numbers() {
             cwd: dirs.test(), h::pipeline(
             r#"
                 open sample.toml
-                | get package.1
+                | get package.name
                 | echo $it
             "#
         ));
 
-        assert_eq!(actual, "0.4.1");
+        assert_eq!(actual, "nu");
     })
 }
 
 #[test]
-fn can_fetch_tables_or_rows_using_numbers_in_column_path() {
+fn column_paths_are_either_double_quoted_or_regular_unquoted_words_separated_by_dot() {
     Playground::setup("get_test_4", |dirs, sandbox| {
         sandbox.with_files(vec![FileWithContent(
             "sample.toml",
             r#"
                 [package]
-                0 = "nu"
+                9999 = ["Yehuda Katz <wycats@gmail.com>", "Jonathan Turner <jonathan.d.turner@gmail.com>", "Andrés N. Robalino <andres@androbtech.com>"]
-                1 = "0.4.1"
-                2 = ["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."
             "#,
         )]);
@@ -95,17 +92,18 @@ fn can_fetch_tables_or_rows_using_numbers_in_column_path() {
             cwd: dirs.test(), h::pipeline(
             r#"
                 open sample.toml
-                | get package.2.1
+                | get package."9999"
+                | count
                 | echo $it
             "#
         ));
 
-        assert_eq!(actual, "Jonathan Turner <jonathan.d.turner@gmail.com>");
+        assert_eq!(actual, "3");
     })
 }
 
 #[test]
-fn fetches_more_than_one_column_member_path() {
+fn fetches_more_than_one_column_path() {
     Playground::setup("get_test_5", |dirs, sandbox| {
         sandbox.with_files(vec![FileWithContent(
             "sample.toml",
@@ -161,9 +159,34 @@ fn errors_fetching_by_column_not_present() {
         assert!(actual.contains("did you mean 'taconushell'?"));
     })
 }
+
 #[test]
-fn errors_fetching_by_index_out_of_bounds_from_table() {
+#[should_panic]
+fn errors_fetching_by_column_using_a_number() {
     Playground::setup("get_test_7", |dirs, sandbox| {
+        sandbox.with_files(vec![FileWithContent(
+            "sample.toml",
+            r#"
+                [spanish_lesson]
+                0 = "can only be fetched with 0 double quoted."
+            "#,
+        )]);
+
+        let actual = nu_error!(
+            cwd: dirs.test(), h::pipeline(
+            r#"
+                open sample.toml
+                | get spanish_lesson.9
+            "#
+        ));
+
+        assert!(actual.contains("No rows available"));
+        assert!(actual.contains(r#"Not a table. Perhaps you meant to get the column "0" instead?"#))
+    })
+}
+#[test]
+fn errors_fetching_by_index_out_of_bounds() {
+    Playground::setup("get_test_8", |dirs, sandbox| {
         sandbox.with_files(vec![FileWithContent(
             "sample.toml",
             r#"
@@ -188,7 +211,7 @@ fn errors_fetching_by_index_out_of_bounds_from_table() {
 
 #[test]
 fn requires_at_least_one_column_member_path() {
-    Playground::setup("get_test_8", |dirs, sandbox| {
+    Playground::setup("get_test_9", |dirs, sandbox| {
         sandbox.with_files(vec![EmptyFile("andres.txt")]);
 
         let actual = nu_error!(