From 6ea8e42331f25df9064c2e9727afded33dab33be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20N=2E=20Robalino?= Date: Fri, 1 Nov 2019 16:19:46 -0500 Subject: [PATCH] Move column paths to support broader value types. --- src/commands/get.rs | 10 +- src/data/base.rs | 126 +++++++----------- src/parser/deserializer.rs | 6 +- .../hir/syntax_shape/expression/number.rs | 19 ++- .../syntax_shape/expression/variable_path.rs | 10 ++ src/parser/hir/tokens_iterator/tests.rs | 16 +++ src/parser/parse/token_tree.rs | 10 ++ src/plugins/edit.rs | 2 +- src/plugins/inc.rs | 20 +-- src/plugins/insert.rs | 13 +- src/plugins/str.rs | 35 +++-- src/utils.rs | 4 +- tests/command_get_tests.rs | 53 +++++--- 13 files changed, 201 insertions(+), 123 deletions(-) create mode 100644 src/parser/hir/tokens_iterator/tests.rs diff --git a/src/commands/get.rs b/src/commands/get.rs index 0176b215d..f5db529c6 100644 --- a/src/commands/get.rs +++ b/src/commands/get.rs @@ -44,7 +44,7 @@ impl WholeStreamCommand for Get { } } -pub type ColumnPath = Vec>; +pub type ColumnPath = Vec>; 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, diff --git a/src/data/base.rs b/src/data/base.rs index 17691e24b..094d425ee 100644 --- a/src/data/base.rs +++ b/src/data/base.rs @@ -409,25 +409,17 @@ impl Tagged { ValueDebug { value: self } } - pub fn as_column_path(&self) -> Result>>, ShellError> { - let mut out: Vec> = vec![]; - + pub fn as_column_path(&self) -> Result>>, ShellError> { match &self.item { - Value::Table(table) => { - for item in table { - out.push(item.as_string()?.tagged(&item.tag)); - } - } - - other => { - return Err(ShellError::type_error( - "column name", - other.type_name().tagged(&self.tag), - )) + Value::Primitive(Primitive::String(s)) => { + Ok(vec![Value::string(s).tagged(&self.tag)].tagged(&self.tag)) } + Value::Table(table) => Ok(table.to_vec().tagged(&self.tag)), + other => Err(ShellError::type_error( + "column name", + other.type_name().tagged(&self.tag), + )), } - - Ok(out.tagged(&self.tag)) } pub(crate) fn as_string(&self) -> Result { @@ -528,30 +520,32 @@ impl Value { pub fn get_data_by_column_path( &self, tag: Tag, - path: &Vec>, - callback: Box)) -> ShellError>, + path: &Vec>, + callback: Box)) -> ShellError>, ) -> Result>, ShellError> { let mut current = self; for p in path { - // note: - // This will eventually be refactored once we are able - // to parse correctly column_paths and get them deserialized - // to values for us. - let value = match p.item().parse::() { - Ok(number) => match current { - Value::Table(_) => current.get_data_by_index(number), - Value::Row(_) => current.get_data_by_key(p), - _ => None, - }, - Err(_) => match self { - Value::Table(_) | Value::Row(_) => current.get_data_by_key(p), - _ => None, - }, - }; // end + let value = match p.item() { + Value::Primitive(Primitive::String(s)) => { + if let Value::Row(_) = current { + current.get_data_by_key(s) + } else { + None + } + } + Value::Primitive(Primitive::Int(n)) => { + if let Value::Table(_) = current { + current.get_data_by_index(n.to_usize().unwrap()) + } else { + None + } + } + _ => None, + }; match value { Some(v) => current = v, - None => return Err(callback((¤t.clone(), &p.clone()))), + None => return Err(callback((current.clone(), p.clone()))), } } @@ -614,9 +608,21 @@ impl Value { pub fn insert_data_at_column_path( &self, tag: Tag, - split_path: &Vec>, + split_path: &Vec>, new_value: Value, ) -> Option> { + 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::>>(); + let mut new_obj = self.clone(); if let Value::Row(ref mut o) = new_obj { @@ -665,14 +671,14 @@ impl Value { pub fn replace_data_at_column_path( &self, tag: Tag, - split_path: &Vec>, + split_path: &Vec>, replaced_value: Value, ) -> Option> { let mut new_obj = self.clone(); let mut current = &mut new_obj; for idx in 0..split_path.len() { - match current.get_mut_data_by_key(&split_path[idx].item) { + match current.get_mut_data_by_key(&split_path[idx].as_string().unwrap()) { Some(next) => { if idx == (split_path.len() - 1) { *next = replaced_value.tagged(&tag); @@ -943,6 +949,10 @@ mod tests { Value::string(input.into()).tagged_unknown() } + fn number(n: i64) -> Tagged { + Value::number(n).tagged_unknown() + } + fn row(entries: IndexMap>) -> Tagged { Value::row(entries).tagged_unknown() } @@ -951,19 +961,12 @@ mod tests { Value::table(list).tagged_unknown() } - fn error_callback() -> impl FnOnce((&Value, &Tagged)) -> ShellError { + fn error_callback() -> impl FnOnce((Value, Tagged)) -> ShellError { move |(_obj_source, _column_path_tried)| ShellError::unimplemented("will never be called.") } - fn column_path(paths: &Vec>) -> Tagged>> { - table( - &paths - .iter() - .map(|p| string(p.as_string().unwrap())) - .collect(), - ) - .as_column_path() - .unwrap() + fn column_path(paths: &Vec>) -> Vec> { + table(paths).as_column_path().unwrap().item } #[test] @@ -1005,36 +1008,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(); diff --git a/src/parser/deserializer.rs b/src/parser/deserializer.rs index 4b8bf913d..c9436ab29 100644 --- a/src/parser/deserializer.rs +++ b/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 - // in the file to special case Tagged parsing. + // Specifically, we rely on this behavior further above + // in the file for the Tagged special case parsing. let tuple = type_name::<()>(); let tagged_tuple = type_name::>(); let tagged_value = type_name::>(); diff --git a/src/parser/hir/syntax_shape/expression/number.rs b/src/parser/hir/syntax_shape/expression/number.rs index 6c599cc02..9a7a6227d 100644 --- a/src/parser/hir/syntax_shape/expression/number.rs +++ b/src/parser/hir/syntax_shape/expression/number.rs @@ -1,7 +1,7 @@ use crate::parser::hir::syntax_shape::{ expand_atom, parse_single_node, ExpandContext, ExpandExpression, ExpansionRule, - FallibleColorSyntax, FlatShape, ParseError, -}; + FallibleColorSyntax, FlatShape, TestSyntax, ParseError}; +use crate::parser::hir::tokens_iterator::Peeked; use crate::parser::{ hir, hir::{RawNumber, TokensIterator}, @@ -212,3 +212,18 @@ impl FallibleColorSyntax for IntShape { Ok(()) } } + +impl TestSyntax for NumberShape { + fn test<'a, 'b>( + &self, + token_nodes: &'b mut TokensIterator<'a>, + _context: &ExpandContext, + ) -> Option> { + let peeked = token_nodes.peek_any(); + + match peeked.node { + Some(token) if token.is_number() => Some(peeked), + _ => None, + } + } +} diff --git a/src/parser/hir/syntax_shape/expression/variable_path.rs b/src/parser/hir/syntax_shape/expression/variable_path.rs index 1a91e132c..f6b4c1931 100644 --- a/src/parser/hir/syntax_shape/expression/variable_path.rs +++ b/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 { diff --git a/src/parser/hir/tokens_iterator/tests.rs b/src/parser/hir/tokens_iterator/tests.rs new file mode 100644 index 000000000..23f888978 --- /dev/null +++ b/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(); +} diff --git a/src/parser/parse/token_tree.rs b/src/parser/parse/token_tree.rs index 75228133d..137b22be7 100644 --- a/src/parser/parse/token_tree.rs +++ b/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 { diff --git a/src/plugins/edit.rs b/src/plugins/edit.rs index 78cb32cef..fb0ac48ed 100644 --- a/src/plugins/edit.rs +++ b/src/plugins/edit.rs @@ -3,7 +3,7 @@ use nu::{ Tagged, Value, }; -pub type ColumnPath = Tagged>>; +pub type ColumnPath = Tagged>>; struct Edit { field: Option, diff --git a/src/plugins/inc.rs b/src/plugins/inc.rs index fb3836dfd..b7b24025e 100644 --- a/src/plugins/inc.rs +++ b/src/plugins/inc.rs @@ -14,7 +14,7 @@ pub enum SemVerAction { Patch, } -pub type ColumnPath = Vec>; +pub type ColumnPath = Tagged>>; struct Inc { field: Option, @@ -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, - Value, + CallInfo, EvaluatedArgs, Plugin, Primitive, ReturnSuccess, Tag, Tagged, TaggedDictBuilder, + TaggedItem, Value, }; struct CallStub { @@ -344,9 +344,13 @@ mod tests { .is_ok()); assert_eq!( - plugin - .field - .map(|f| f.iter().map(|f| f.item.clone()).collect()), + plugin.field.map(|f| f + .iter() + .map(|f| match &f.item { + Value::Primitive(Primitive::String(s)) => s.clone(), + _ => panic!(""), + }) + .collect()), Some(vec!["package".to_string(), "version".to_string()]) ); } diff --git a/src/plugins/insert.rs b/src/plugins/insert.rs index 9041abc36..cfe3a27b5 100644 --- a/src/plugins/insert.rs +++ b/src/plugins/insert.rs @@ -4,7 +4,7 @@ use nu::{ Tagged, TaggedItem, Value, }; -pub type ColumnPath = Vec>; +pub type ColumnPath = Vec>; struct Insert { field: Option, @@ -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, diff --git a/src/plugins/str.rs b/src/plugins/str.rs index 7fb694d3a..552602acc 100644 --- a/src/plugins/str.rs +++ b/src/plugins/str.rs @@ -12,7 +12,7 @@ enum Action { Substring(usize, usize), } -pub type ColumnPath = Vec>; +pub type ColumnPath = Tagged>>; struct Str { field: Option, @@ -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,22 @@ impl Plugin for Str { if let Some(possible_field) = args.nth(0) { match possible_field { - Tagged { - item: Value::Primitive(Primitive::String(s)), - tag, - } => { - self.for_field(vec![s.clone().tagged(tag)]); - } + string @ Tagged { + item: Value::Primitive(Primitive::String(_)), + .. + } => match self.action { + Some(Action::Downcase) + | Some(Action::Upcase) + | Some(Action::ToInteger) + | None => { + 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 +424,13 @@ mod tests { .is_ok()); assert_eq!( - plugin - .field - .map(|f| f.into_iter().map(|f| f.item).collect()), + plugin.field.map(|f| f + .iter() + .map(|f| match &f.item { + Value::Primitive(Primitive::String(s)) => s.clone(), + _ => panic!(""), + }) + .collect()), Some(vec!["package".to_string(), "description".to_string()]) ) } diff --git a/src/utils.rs b/src/utils.rs index 9822b7627..cef4aad19 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -7,8 +7,10 @@ use std::path::{Component, Path, PathBuf}; pub fn did_you_mean( obj_source: &Value, - field_tried: &Tagged, + field_tried: &Tagged, ) -> Option> { + let field_tried = field_tried.as_string().unwrap(); + let possibilities = obj_source.data_descriptors(); let mut possible_matches: Vec<_> = possibilities diff --git a/tests/command_get_tests.rs b/tests/command_get_tests.rs index 09348678b..386b1795e 100644 --- a/tests/command_get_tests.rs +++ b/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" - 1 = "0.4.1" + name = "nu" "#, )]); @@ -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" - 1 = "0.4.1" - 2 = ["Yehuda Katz ", "Jonathan Turner ", "Andrés N. Robalino "] + 9999 = ["Yehuda Katz ", "Jonathan Turner ", "Andrés N. Robalino "] 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 "); + 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() { +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.0 + "# + )); + + assert!(actual.contains("No rows available")); + assert!(actual.contains("Tried getting a row indexed at '0'")); + 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!(