From 4b83a2d27a99a3b53e4397de7b649c49822b84b2 Mon Sep 17 00:00:00 2001 From: Leon Date: Sun, 20 Nov 2022 03:35:55 +1000 Subject: [PATCH] Improve CantFindColumn and ColumnAlreadyExists errors (#7164) * Improve CantFindColumn and ColumnAlreadyExists errors * Update tests --- crates/nu-command/src/filters/group_by.rs | 1 + crates/nu-command/src/filters/split_by.rs | 2 + crates/nu-command/src/sort_utils.rs | 16 ++- crates/nu-command/tests/commands/insert.rs | 4 +- crates/nu-command/tests/commands/update.rs | 10 ++ crates/nu-engine/src/column.rs | 17 +--- crates/nu-protocol/src/shell_error.rs | 6 +- crates/nu-protocol/src/value/mod.rs | 112 +++++++++++++++++---- 8 files changed, 131 insertions(+), 37 deletions(-) diff --git a/crates/nu-command/src/filters/group_by.rs b/crates/nu-command/src/filters/group_by.rs index 477139e84..617e725fe 100644 --- a/crates/nu-command/src/filters/group_by.rs +++ b/crates/nu-command/src/filters/group_by.rs @@ -250,6 +250,7 @@ pub fn group( move |_, row: &Value| match row.get_data_by_key(&column_name.item) { Some(group_key) => Ok(group_key.as_string()?), None => Err(ShellError::CantFindColumn( + column_name.item.to_string(), column_name.span, row.span().unwrap_or(column_name.span), )), diff --git a/crates/nu-command/src/filters/split_by.rs b/crates/nu-command/src/filters/split_by.rs index 93b328feb..d42f89fe6 100644 --- a/crates/nu-command/src/filters/split_by.rs +++ b/crates/nu-command/src/filters/split_by.rs @@ -137,6 +137,7 @@ pub fn split_by( }); Ok(split(&splitter, input, name)?) } + // This uses the same format as the 'requires a column name' error in sort_utils.rs None => Err(ShellError::GenericError( "expected name".into(), "requires a column name for splitting".into(), @@ -165,6 +166,7 @@ pub fn split( move |_, row: &Value| match row.get_data_by_key(&column_name.item) { Some(group_key) => Ok(group_key.as_string()?), None => Err(ShellError::CantFindColumn( + column_name.item.to_string(), column_name.span, row.span().unwrap_or(column_name.span), )), diff --git a/crates/nu-command/src/sort_utils.rs b/crates/nu-command/src/sort_utils.rs index 9189a2c79..f504b0166 100644 --- a/crates/nu-command/src/sort_utils.rs +++ b/crates/nu-command/src/sort_utils.rs @@ -1,5 +1,5 @@ use alphanumeric_sort::compare_str; -use nu_engine::column::column_does_not_exist; +use nu_engine::column::nonexistent_column; use nu_protocol::{ShellError, Span, Value}; use std::cmp::Ordering; @@ -81,12 +81,18 @@ pub fn sort( .. } => { if sort_columns.is_empty() { - println!("sort-by requires a column name to sort table data"); - return Err(ShellError::CantFindColumn(span, span)); + // This uses the same format as the 'requires a column name' error in split_by.rs + return Err(ShellError::GenericError( + "expected name".into(), + "requires a column name to sort table data".into(), + Some(span), + None, + Vec::new(), + )); } - if column_does_not_exist(sort_columns.clone(), cols.to_vec()) { - return Err(ShellError::CantFindColumn(span, span)); + if let Some(nonexistent) = nonexistent_column(sort_columns.clone(), cols.to_vec()) { + return Err(ShellError::CantFindColumn(nonexistent, span, span)); } // check to make sure each value in each column in the record diff --git a/crates/nu-command/tests/commands/insert.rs b/crates/nu-command/tests/commands/insert.rs index 38c3d429f..5ac10f6fa 100644 --- a/crates/nu-command/tests/commands/insert.rs +++ b/crates/nu-command/tests/commands/insert.rs @@ -24,7 +24,9 @@ fn insert_the_column_conflict() { "# )); - assert!(actual.err.contains("column already exists")); + assert!(actual + .err + .contains("column 'pretty_assertions' already exists")); } #[test] diff --git a/crates/nu-command/tests/commands/update.rs b/crates/nu-command/tests/commands/update.rs index bbbffa6ea..63fef7dc4 100644 --- a/crates/nu-command/tests/commands/update.rs +++ b/crates/nu-command/tests/commands/update.rs @@ -95,3 +95,13 @@ fn update_past_end_list() { assert!(actual.err.contains("too large")); } + +#[test] +fn update_nonexistent_column() { + let actual = nu!( + cwd: ".", pipeline( + r#"{a:1} | update b 2"# + )); + + assert!(actual.err.contains("cannot find column 'b'")); +} diff --git a/crates/nu-engine/src/column.rs b/crates/nu-engine/src/column.rs index 31d0d0230..c77043a1d 100644 --- a/crates/nu-engine/src/column.rs +++ b/crates/nu-engine/src/column.rs @@ -19,22 +19,15 @@ pub fn get_columns<'a>(input: impl IntoIterator) -> Vec, columns: Vec) -> bool { - let mut set = HashSet::new(); - for column in columns { - set.insert(column); - } +// If a column doesn't exist in the input, return it. +pub fn nonexistent_column(inputs: Vec, columns: Vec) -> Option { + let set: HashSet = HashSet::from_iter(columns.iter().cloned()); for input in &inputs { if set.contains(input) { continue; } - return true; + return Some(input.clone()); } - false + None } diff --git a/crates/nu-protocol/src/shell_error.rs b/crates/nu-protocol/src/shell_error.rs index 018aa91c7..42fc2552a 100644 --- a/crates/nu-protocol/src/shell_error.rs +++ b/crates/nu-protocol/src/shell_error.rs @@ -411,7 +411,8 @@ Either make sure {0} is a string, or add a 'to_string' entry for it in ENV_CONVE #[error("Cannot find column")] #[diagnostic(code(nu::shell::column_not_found), url(docsrs))] CantFindColumn( - #[label = "cannot find column"] Span, + String, + #[label = "cannot find column '{0}'"] Span, #[label = "value originates here"] Span, ), @@ -423,7 +424,8 @@ Either make sure {0} is a string, or add a 'to_string' entry for it in ENV_CONVE #[error("Column already exists")] #[diagnostic(code(nu::shell::column_already_exists), url(docsrs))] ColumnAlreadyExists( - #[label = "column already exists"] Span, + String, + #[label = "column '{0}' already exists"] Span, #[label = "value originates here"] Span, ), diff --git a/crates/nu-protocol/src/value/mod.rs b/crates/nu-protocol/src/value/mod.rs index cb879b87e..b4ccb6e44 100644 --- a/crates/nu-protocol/src/value/mod.rs +++ b/crates/nu-protocol/src/value/mod.rs @@ -718,7 +718,11 @@ impl Value { return Err(ShellError::DidYouMean(suggestion, *origin_span)); } } - return Err(ShellError::CantFindColumn(*origin_span, span)); + return Err(ShellError::CantFindColumn( + column_name.to_string(), + *origin_span, + span, + )); } } Value::List { vals, span } => { @@ -826,7 +830,13 @@ impl Value { } } } - v => return Err(ShellError::CantFindColumn(*span, v.span()?)), + v => { + return Err(ShellError::CantFindColumn( + col_name.to_string(), + *span, + v.span()?, + )) + } } } } @@ -856,7 +866,13 @@ impl Value { } } } - v => return Err(ShellError::CantFindColumn(*span, v.span()?)), + v => { + return Err(ShellError::CantFindColumn( + col_name.to_string(), + *span, + v.span()?, + )) + } }, PathMember::Int { val: row_num, span } => match self { Value::List { vals, .. } => { @@ -924,10 +940,20 @@ impl Value { } } if !found { - return Err(ShellError::CantFindColumn(*span, *v_span)); + return Err(ShellError::CantFindColumn( + col_name.to_string(), + *span, + *v_span, + )); } } - v => return Err(ShellError::CantFindColumn(*span, v.span()?)), + v => { + return Err(ShellError::CantFindColumn( + col_name.to_string(), + *span, + v.span()?, + )) + } } } } @@ -947,10 +973,20 @@ impl Value { } } if !found { - return Err(ShellError::CantFindColumn(*span, *v_span)); + return Err(ShellError::CantFindColumn( + col_name.to_string(), + *span, + *v_span, + )); } } - v => return Err(ShellError::CantFindColumn(*span, v.span()?)), + v => { + return Err(ShellError::CantFindColumn( + col_name.to_string(), + *span, + v.span()?, + )) + } }, PathMember::Int { val: row_num, span } => match self { Value::List { vals, .. } => { @@ -999,10 +1035,20 @@ impl Value { } } if !found { - return Err(ShellError::CantFindColumn(*span, *v_span)); + return Err(ShellError::CantFindColumn( + col_name.to_string(), + *span, + *v_span, + )); } } - v => return Err(ShellError::CantFindColumn(*span, v.span()?)), + v => { + return Err(ShellError::CantFindColumn( + col_name.to_string(), + *span, + v.span()?, + )) + } } } Ok(()) @@ -1021,11 +1067,19 @@ impl Value { } } if !found { - return Err(ShellError::CantFindColumn(*span, *v_span)); + return Err(ShellError::CantFindColumn( + col_name.to_string(), + *span, + *v_span, + )); } Ok(()) } - v => Err(ShellError::CantFindColumn(*span, v.span()?)), + v => Err(ShellError::CantFindColumn( + col_name.to_string(), + *span, + v.span()?, + )), }, PathMember::Int { val: row_num, span } => match self { Value::List { vals, .. } => { @@ -1065,10 +1119,20 @@ impl Value { } } if !found { - return Err(ShellError::CantFindColumn(*span, *v_span)); + return Err(ShellError::CantFindColumn( + col_name.to_string(), + *span, + *v_span, + )); } } - v => return Err(ShellError::CantFindColumn(*span, v.span()?)), + v => { + return Err(ShellError::CantFindColumn( + col_name.to_string(), + *span, + v.span()?, + )) + } } } Ok(()) @@ -1088,11 +1152,19 @@ impl Value { } } if !found { - return Err(ShellError::CantFindColumn(*span, *v_span)); + return Err(ShellError::CantFindColumn( + col_name.to_string(), + *span, + *v_span, + )); } Ok(()) } - v => Err(ShellError::CantFindColumn(*span, v.span()?)), + v => Err(ShellError::CantFindColumn( + col_name.to_string(), + *span, + v.span()?, + )), }, PathMember::Int { val: row_num, span } => match self { Value::List { vals, .. } => { @@ -1134,7 +1206,9 @@ impl Value { if col.0 == col_name { if cell_path.len() == 1 { return Err(ShellError::ColumnAlreadyExists( - *span, *v_span, + col_name.to_string(), + *span, + *v_span, )); } else { return col.1.insert_data_at_cell_path( @@ -1165,7 +1239,11 @@ impl Value { for col in cols.iter().zip(vals.iter_mut()) { if col.0 == col_name { if cell_path.len() == 1 { - return Err(ShellError::ColumnAlreadyExists(*span, *v_span)); + return Err(ShellError::ColumnAlreadyExists( + col_name.to_string(), + *span, + *v_span, + )); } else { return col .1