Get error message improvements. (#1185)

More especific "get" command error messages + Test refactoring.

crates/nu-source/src/meta.rs

@@ -393,7 +393,6 @@ impl Tag {
     }
 }
 
-#[allow(unused)]
 pub fn tag_for_tagged_list(mut iter: impl Iterator<Item = Tag>) -> Tag {
     let first = iter.next();
 
@@ -410,7 +409,6 @@ pub fn tag_for_tagged_list(mut iter: impl Iterator<Item = Tag>) -> Tag {
     }
 }
 
-#[allow(unused)]
 pub fn span_for_spanned_list(mut iter: impl Iterator<Item = Span>) -> Span {
     let first = iter.next();
 
@@ -471,6 +469,12 @@ impl Span {
         }
     }
 
+    pub fn since(&self, other: impl Into<Span>) -> Span {
+        let other = other.into();
+
+        Span::new(other.start, self.end)
+    }
+
     pub fn until(&self, other: impl Into<Span>) -> Span {
         let other = other.into();
 

src/commands/get.rs

@@ -2,13 +2,14 @@ use crate::commands::WholeStreamCommand;
 use crate::data::base::shape::Shapes;
 use crate::prelude::*;
 use futures_util::pin_mut;
+use indexmap::set::IndexSet;
 use log::trace;
 use nu_errors::ShellError;
 use nu_protocol::{
-    did_you_mean, ColumnPath, ReturnSuccess, ReturnValue, Signature, SyntaxShape, UntaggedValue,
+    did_you_mean, ColumnPath, PathMember, ReturnSuccess, ReturnValue, Signature, SyntaxShape,
-    Value,
+    UnspannedPathMember, UntaggedValue, Value,
 };
-use nu_source::{span_for_spanned_list, PrettyDebug};
+use nu_source::span_for_spanned_list;
 use nu_value_ext::get_data_by_column_path;
 
 pub struct Get;
@@ -50,21 +51,64 @@ pub fn get_column_path(path: &ColumnPath, obj: &Value) -> Result<Value, ShellErr
         obj,
         path,
         Box::new(move |(obj_source, column_path_tried, error)| {
-            if let UntaggedValue::Table(rows) = &obj_source.value {
+            let path_members_span = span_for_spanned_list(fields.members().iter().map(|p| p.span));
-                let total = rows.len();
-                let end_tag = match fields
-                    .members()
-                    .iter()
-                    .nth_back(if fields.members().len() > 2 { 1 } else { 0 })
-                {
-                    Some(last_field) => last_field.span,
-                    None => column_path_tried.span,
-                };
 
-                let primary_label = format!(
+            match &obj_source.value {
-                    "There isn't a row indexed at {}",
+                UntaggedValue::Table(rows) => match column_path_tried {
-                    column_path_tried.display()
+                    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()
@@ -74,18 +118,58 @@ pub fn get_column_path(path: &ColumnPath, obj: &Value) -> Result<Value, ShellErr
 
                         return ShellError::labeled_error_with_secondary(
                             "Row not found",
-                    primary_label,
+                            format!("There isn't a row indexed at {}", idx),
                             column_path_tried.span,
                             secondary_label,
-                    end_tag,
+                            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),
-                    span_for_spanned_list(fields.members().iter().map(|p| p.span)),
+                    column_path_tried.span.since(path_members_span),
                 );
             }
 

src/utils.rs

@@ -316,18 +316,10 @@ mod tests {
                     loc: fixtures().join("cargo_sample.toml"),
                     at: 0
                 },
-                Res {
-                    loc: fixtures().join("fileA.txt"),
-                    at: 0
-                },
                 Res {
                     loc: fixtures().join("jonathan.xml"),
                     at: 0
                 },
-                Res {
-                    loc: fixtures().join("nested_uniq.json"),
-                    at: 0
-                },
                 Res {
                     loc: fixtures().join("sample.bson"),
                     at: 0

tests/commands/append.rs

@@ -1,17 +1,30 @@
+use nu_test_support::fs::Stub::FileWithContentToBeTrimmed;
+use nu_test_support::playground::Playground;
 use nu_test_support::{nu, pipeline};
 
 #[test]
 fn adds_a_row_to_the_end() {
-    let actual = nu!(
+    Playground::setup("append_test_1", |dirs, sandbox| {
-        cwd: "tests/fixtures/formats", pipeline(
+        sandbox.with_files(vec![FileWithContentToBeTrimmed(
+            "los_tres_caballeros.txt",
             r#"
-            open fileA.txt
+                Andrés N. Robalino
+                Jonathan Turner
+                Yehuda Katz
+            "#,
+        )]);
+
+        let actual = nu!(
+            cwd: dirs.test(), pipeline(
+            r#"
+                open los_tres_caballeros.txt
                 | lines
-            | append "testme"
+                | append "pollo loco"
                 | nth 3
                 | echo $it
             "#
         ));
 
-    assert_eq!(actual, "testme");
+        assert_eq!(actual, "pollo loco");
+    })
 }

tests/commands/get.rs

@@ -154,13 +154,22 @@ fn errors_fetching_by_column_not_present() {
             "#
         ));
 
-        assert!(actual.contains("Unknown column"));
+        assert!(
-        assert!(actual.contains("did you mean 'taconushell'?"));
+            actual.contains("Unknown column"),
+            format!("actual: {:?}", actual)
+        );
+        assert!(
+            actual.contains("There isn't a column named 'taco'"),
+            format!("actual: {:?}", actual)
+        );
+        assert!(
+            actual.contains("Perhaps you meant 'taconushell'?"),
+            format!("actual: {:?}", actual)
+        )
     })
 }
 
 #[test]
-#[should_panic]
 fn errors_fetching_by_column_using_a_number() {
     Playground::setup("get_test_7", |dirs, sandbox| {
         sandbox.with_files(vec![FileWithContent(
@@ -175,12 +184,22 @@ fn errors_fetching_by_column_using_a_number() {
             cwd: dirs.test(), pipeline(
             r#"
                 open sample.toml
-                | get spanish_lesson.9
+                | get spanish_lesson.0
             "#
         ));
 
-        assert!(actual.contains("No rows available"));
+        assert!(
-        assert!(actual.contains(r#"Not a table. Perhaps you meant to get the column "0" instead?"#))
+            actual.contains("No rows available"),
+            format!("actual: {:?}", actual)
+        );
+        assert!(
+            actual.contains("A row at '0' can't be indexed."),
+            format!("actual: {:?}", actual)
+        );
+        assert!(
+            actual.contains("Appears to contain columns. Columns available: 0"),
+            format!("actual: {:?}", actual)
+        )
     })
 }
 #[test]

tests/commands/mod.rs

@@ -18,6 +18,7 @@ mod mkdir;
 mod mv;
 mod open;
 mod parse;
+mod pick;
 mod prepend;
 mod range;
 mod reverse;

tests/commands/parse.rs

@@ -1,11 +1,23 @@
+use nu_test_support::fs::Stub::FileWithContentToBeTrimmed;
+use nu_test_support::playground::Playground;
 use nu_test_support::{nu, pipeline};
 
 #[test]
 fn extracts_fields_from_the_given_the_pattern() {
-    let actual = nu!(
+    Playground::setup("parse_test_1", |dirs, sandbox| {
-        cwd: "tests/fixtures/formats", pipeline(
+        sandbox.with_files(vec![FileWithContentToBeTrimmed(
+            "key_value_separated_arepa_ingredients.txt",
             r#"
-            open fileA.txt
+                VAR1=Cheese
+                VAR2=JonathanParsed
+                VAR3=NushellSecretIngredient
+            "#,
+        )]);
+
+        let actual = nu!(
+            cwd: dirs.test(), pipeline(
+            r#"
+                open key_value_separated_arepa_ingredients.txt
                 | parse "{Name}={Value}"
                 | nth 1
                 | get Value
@@ -13,5 +25,6 @@ fn extracts_fields_from_the_given_the_pattern() {
             "#
         ));
 
-    assert_eq!(actual, "StupidLongName");
+        assert_eq!(actual, "JonathanParsed");
+    })
 }

tests/commands/pick.rs

@@ -0,0 +1,57 @@
+use nu_test_support::fs::Stub::FileWithContentToBeTrimmed;
+use nu_test_support::playground::Playground;
+use nu_test_support::{nu, nu_error, pipeline};
+
+#[test]
+fn columns() {
+    Playground::setup("pick_by_test_1", |dirs, sandbox| {
+        sandbox.with_files(vec![FileWithContentToBeTrimmed(
+            "los_tres_caballeros.csv",
+            r#"
+                first_name,last_name,rusty_at,type
+                Andrés,Robalino,10/11/2013,A
+                Jonathan,Turner,10/12/2013,B
+                Yehuda,Katz,10/11/2013,A
+            "#,
+        )]);
+
+        let actual = nu!(
+            cwd: dirs.test(), pipeline(
+            r#"
+                open los_tres_caballeros.csv
+                | pick rusty_at last_name
+                | nth 0
+                | get last_name
+                | echo $it
+            "#
+        ));
+
+        assert_eq!(actual, "Robalino");
+    })
+}
+
+#[should_panic]
+#[test]
+fn errors_if_given_unknown_column_name_is_missing() {
+    Playground::setup("pick_test_2", |dirs, sandbox| {
+        sandbox.with_files(vec![FileWithContentToBeTrimmed(
+            "los_tres_caballeros.csv",
+            r#"
+                first_name,last_name,rusty_at,type
+                Andrés,Robalino,10/11/2013,A
+                Jonathan,Turner,10/12/2013,B
+                Yehuda,Katz,10/11/2013,A
+            "#,
+        )]);
+
+        let actual = nu_error!(
+            cwd: dirs.test(), pipeline(
+            r#"
+                open los_tres_caballeros.csv
+                | pick rrusty_at
+            "#
+        ));
+
+        assert!(actual.contains("Unknown column"));
+    })
+}

tests/commands/prepend.rs

@@ -1,17 +1,30 @@
+use nu_test_support::fs::Stub::FileWithContentToBeTrimmed;
+use nu_test_support::playground::Playground;
 use nu_test_support::{nu, pipeline};
 
 #[test]
 fn adds_a_row_to_the_beginning() {
-    let actual = nu!(
+    Playground::setup("prepend_test_1", |dirs, sandbox| {
-        cwd: "tests/fixtures/formats", pipeline(
+        sandbox.with_files(vec![FileWithContentToBeTrimmed(
+            "los_tres_caballeros.txt",
             r#"
-            open fileA.txt
+                Andrés N. Robalino
+                Jonathan Turner
+                Yehuda Katz
+            "#,
+        )]);
+
+        let actual = nu!(
+            cwd: dirs.test(), pipeline(
+            r#"
+                open los_tres_caballeros.txt
                 | lines
-            | prepend "testme"
+                | prepend "pollo loco"
                 | nth 0
                 | echo $it
                 "#
         ));
 
-    assert_eq!(actual, "testme");
+        assert_eq!(actual, "pollo loco");
+    })
 }

tests/commands/uniq.rs

@@ -3,7 +3,7 @@ use nu_test_support::playground::Playground;
 use nu_test_support::{nu, pipeline};
 
 #[test]
-fn uniq_rows() {
+fn removes_duplicate_rows() {
     Playground::setup("uniq_test_1", |dirs, sandbox| {
         sandbox.with_files(vec![FileWithContentToBeTrimmed(
             "los_tres_caballeros.csv",
@@ -31,39 +31,9 @@ fn uniq_rows() {
     })
 }
 
-#[test]
-fn uniq_columns() {
-    Playground::setup("uniq_test_2", |dirs, sandbox| {
-        sandbox.with_files(vec![FileWithContentToBeTrimmed(
-            "los_tres_caballeros.csv",
-            r#"
-                first_name,last_name,rusty_at,type
-                Andrés,Robalino,10/11/2013,A
-                Jonathan,Turner,10/12/2013,B
-                Yehuda,Katz,10/11/2013,A
-                Jonathan,Turner,10/12/2013,B
-                Yehuda,Katz,10/11/2013,A
-            "#,
-        )]);
-
-        let actual = nu!(
-            cwd: dirs.test(), pipeline(
-            r#"
-                open los_tres_caballeros.csv
-                | pick rusty_at type
-                | uniq
-                | count
-                | echo $it
-            "#
-        ));
-
-        assert_eq!(actual, "2");
-    })
-}
-
 #[test]
 fn uniq_values() {
-    Playground::setup("uniq_test_3", |dirs, sandbox| {
+    Playground::setup("uniq_test_2", |dirs, sandbox| {
         sandbox.with_files(vec![FileWithContentToBeTrimmed(
             "los_tres_caballeros.csv",
             r#"
@@ -91,6 +61,70 @@ fn uniq_values() {
     })
 }
 
+#[test]
+fn nested_json_structures() {
+    Playground::setup("uniq_test_3", |dirs, sandbox| {
+        sandbox.with_files(vec![FileWithContentToBeTrimmed(
+            "nested_json_structures.json",
+            r#"
+            [ 
+                {
+                  "name": "this is duplicated",
+                  "nesting": [ { "a": "a", "b": "b" },
+                               { "c": "c", "d": "d" }
+                  ],
+                  "can_be_ordered_differently": {
+                    "array": [1, 2, 3, 4, 5],
+                    "something": { "else": "works" }
+                  }
+                },
+                {
+                  "can_be_ordered_differently": {
+                    "something": { "else": "works" },
+                    "array": [1, 2, 3, 4, 5]
+                  },
+                  "nesting": [ { "b": "b", "a": "a" },
+                               { "d": "d", "c": "c" }
+                  ],
+                  "name": "this is duplicated"
+                },
+                {
+                  "name": "this is unique",
+                  "nesting": [ { "a": "b", "b": "a" },
+                               { "c": "d", "d": "c" }
+                  ],
+                  "can_be_ordered_differently": {
+                    "array": [],
+                    "something": { "else": "does not work" }
+                  }
+                },
+                {
+                  "name": "this is unique",
+                  "nesting": [ { "a": "a", "b": "b", "c": "c" },
+                               { "d": "d", "e": "e", "f": "f" }
+                  ],
+                  "can_be_ordered_differently": {
+                    "array": [],
+                    "something": { "else": "works" }
+                  }
+                }
+              ]
+            "#,
+        )]);
+
+        let actual = nu!(
+            cwd: dirs.test(), pipeline(
+            r#"
+                open nested_json_structures.json
+                | uniq
+                | count
+                | echo $it
+            "#
+        ));
+        assert_eq!(actual, "3");
+    })
+}
+
 #[test]
 fn uniq_when_keys_out_of_order() {
     let actual = nu!(
@@ -106,13 +140,3 @@ fn uniq_when_keys_out_of_order() {
 
     assert_eq!(actual, "1");
 }
-
-#[test]
-fn uniq_nested_json_structures() {
-    let actual = nu!(
-        cwd: "tests/fixtures/formats",
-        "open nested_uniq.json | uniq | count | echo $it"
-    );
-
-    assert_eq!(actual, "3");
-}

tests/commands/where_.rs

@@ -7,7 +7,7 @@ fn filters_by_unit_size_comparison() {
         "ls | where size > 1kb | sort-by size | get name | first 1 | trim | echo $it"
     );
 
-    assert_eq!(actual, "nested_uniq.json");
+    assert_eq!(actual, "cargo_sample.toml");
 }
 
 #[test]

tests/filters.rs

@@ -1,43 +0,0 @@
-// use nu_test_support::{nu, pipeline};
-// use nu_test_support::playground::Playground;
-// use nu_test_support::fs::Stub::FileWithContentToBeTrimmed;
-
-// #[test]
-// fn can_sum() {
-//     let actual = nu!(
-//         cwd: "tests/fixtures/formats", h::pipeline(
-//         r#"
-//             open sgml_description.json
-//             | get glossary.GlossDiv.GlossList.GlossEntry.Sections
-//             | sum
-//             | echo $it
-//         "#
-//     ));
-
-//     assert_eq!(actual, "203")
-// }
-
-// #[test]
-// fn can_average_numbers() {
-//     let actual = nu!(
-//         cwd: "tests/fixtures/formats", h::pipeline(
-//         r#"
-//             open sgml_description.json
-//             | get glossary.GlossDiv.GlossList.GlossEntry.Sections
-//             | average
-//             | echo $it
-//         "#
-//     ));
-
-//     assert_eq!(actual, "101.5000000000000")
-// }
-
-// #[test]
-// fn can_average_bytes() {
-//     let actual = nu!(
-//         cwd: "tests/fixtures/formats",
-//         "ls | sort-by name | skip 1 | first 2 | get size | average | echo $it"
-//     );
-
-//     assert_eq!(actual, "1600.000000000000");
-// }

tests/fixtures/formats/fileA.txt

@@ -1,3 +0,0 @@
-VAR1=Chill
-VAR2=StupidLongName
-VAR3=AlsoChill

tests/fixtures/formats/nested_uniq.json

@@ -1,72 +0,0 @@
-[ 
-  {
-    "name": "this is duplicated",
-    "nesting": [
-      {
-        "a": "a",
-        "b": "b"
-      },
-      {
-        "c": "c",
-        "d": "d"
-      }
-    ],
-    "can_be_ordered_differently": {
-      "array": [1, 2, 3, 4, 5],
-      "something": { "else": "works" }
-    }
-  },
-  {
-    "can_be_ordered_differently": {
-      "something": { "else": "works" },
-      "array": [1, 2, 3, 4, 5]
-    },
-    "nesting": [
-      {
-        "b": "b",
-        "a": "a"
-      },
-      {
-        "d": "d",
-        "c": "c"
-      }
-    ],
-    "name": "this is duplicated"
-  },
-  {
-    "name": "this is unique",
-    "nesting": [
-      {
-        "a": "b",
-        "b": "a"
-      },
-      {
-        "c": "d",
-        "d": "c"
-      }
-    ],
-    "can_be_ordered_differently": {
-      "array": [],
-      "something": { "else": "does not work" }
-    }
-  },
-  {
-    "name": "this is unique",
-    "nesting": [
-      {
-        "a": "a",
-        "b": "b",
-        "c": "c"
-      },
-      {
-        "d": "d",
-        "e": "e",
-        "f": "f"
-      }
-    ],
-    "can_be_ordered_differently": {
-      "array": [],
-      "something": { "else": "works" }
-    }
-  }
-]