From b12ffb88886de062dbf1b3d8bd736aa26f74910b Mon Sep 17 00:00:00 2001 From: Leon Date: Wed, 23 Nov 2022 16:22:23 +1000 Subject: [PATCH] Fix `sort-by`, `path join` and `size` error arrows (#7199) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description BEFORE: ``` 〉ls | size Error: nu::shell::pipeline_mismatch (link) × Pipeline mismatch. ╭─[entry #22:1:1] 1 │ ls | size · ──┬─ · │╰── value originates from here · ╰── expected: string ╰──── 〉ls | sort-by SIZE Error: nu::shell::column_not_found (link) × Cannot find column ╭─[entry #17:1:1] 1 │ ls | sort-by SIZE · ───┬─── · │╰── value originates here · ╰── cannot find column ╰──── 〉[4kb] | path join 'b' Error: nu::shell::pipeline_mismatch (link) × Pipeline mismatch. ╭─[entry #6:1:1] 1 │ [4kb] | path join 'b' · ──┬── · │╰── value originates from here · ╰── expected: string or record ╰──── ``` AFTER: ``` 〉ls | size Error: nu::shell::pipeline_mismatch (link) × Pipeline mismatch. ╭─[entry #1:1:1] 1 │ ls | size · ─┬ ──┬─ · │ ╰── expected: string · ╰── value originates from here ╰──── 〉ls | get 0 | sort-by SIZE Error: nu::shell::column_not_found (link) × Cannot find column ╭─[entry #2:1:1] 1 │ ls | get 0 | sort-by SIZE · ─┬ ───┬─── · │ ╰── cannot find column 'SIZE' · ╰── value originates here ╰──── 〉[4kb] | path join 'b' Error: nu::shell::pipeline_mismatch (link) × Pipeline mismatch. ╭─[entry #1:1:1] 1 │ [4kb] | path join 'b' · ──┬── ────┬──── · │ ╰── expected: string or record · ╰── value originates from here ╰──── ``` (Hey, anyone noticed that there's TWO wordings of "value originates from here" in this codebase………?) # User-Facing Changes See above. # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style - `cargo test --workspace --features=extra` to check that all tests pass # After Submitting If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --- crates/nu-command/src/path/join.rs | 6 +++--- crates/nu-command/src/sort_utils.rs | 4 ++-- crates/nu-command/src/strings/size.rs | 18 +++++++++++++----- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/crates/nu-command/src/path/join.rs b/crates/nu-command/src/path/join.rs index 51cade4c1a..94d8179b93 100644 --- a/crates/nu-command/src/path/join.rs +++ b/crates/nu-command/src/path/join.rs @@ -158,7 +158,7 @@ fn handle_value(v: Value, args: &Arguments, head: Span) -> Value { match v { Value::String { ref val, span } => join_single(Path::new(val), span, args), Value::Record { cols, vals, span } => join_record(&cols, &vals, span, args), - Value::List { vals, span } => join_list(&vals, span, args), + Value::List { vals, span } => join_list(&vals, span, head, args), _ => super::handle_invalid_values(v, head), } @@ -173,7 +173,7 @@ fn join_single(path: &Path, span: Span, args: &Arguments) -> Value { Value::string(result.to_string_lossy(), span) } -fn join_list(parts: &[Value], span: Span, args: &Arguments) -> Value { +fn join_list(parts: &[Value], span: Span, head: Span, args: &Arguments) -> Value { let path: Result = parts.iter().map(Value::as_string).collect(); match path { @@ -190,7 +190,7 @@ fn join_list(parts: &[Value], span: Span, args: &Arguments) -> Value { Value::List { vals, span } } Err(_) => Value::Error { - error: ShellError::PipelineMismatch("string or record".into(), span, span), + error: ShellError::PipelineMismatch("string or record".into(), head, span), }, } } diff --git a/crates/nu-command/src/sort_utils.rs b/crates/nu-command/src/sort_utils.rs index f504b01668..00b597d126 100644 --- a/crates/nu-command/src/sort_utils.rs +++ b/crates/nu-command/src/sort_utils.rs @@ -78,7 +78,7 @@ pub fn sort( Value::Record { cols, vals: _input_vals, - .. + span: val_span, } => { if sort_columns.is_empty() { // This uses the same format as the 'requires a column name' error in split_by.rs @@ -92,7 +92,7 @@ pub fn sort( } if let Some(nonexistent) = nonexistent_column(sort_columns.clone(), cols.to_vec()) { - return Err(ShellError::CantFindColumn(nonexistent, span, span)); + return Err(ShellError::CantFindColumn(nonexistent, span, *val_span)); } // check to make sure each value in each column in the record diff --git a/crates/nu-command/src/strings/size.rs b/crates/nu-command/src/strings/size.rs index 0069e233ca..3f89e8505b 100644 --- a/crates/nu-command/src/strings/size.rs +++ b/crates/nu-command/src/strings/size.rs @@ -162,11 +162,19 @@ fn size( ) -> Result { let span = call.head; input.map( - move |v| match v.as_string() { - Ok(s) => counter(&s, span), - Err(_) => Value::Error { - error: ShellError::PipelineMismatch("string".into(), span, span), - }, + move |v| { + // First, obtain the span. If this fails, propagate the error that results. + let value_span = match v.span() { + Err(v) => return Value::Error { error: v }, + Ok(v) => v, + }; + // Now, check if it's a string. + match v.as_string() { + Ok(s) => counter(&s, span), + Err(_) => Value::Error { + error: ShellError::PipelineMismatch("string".into(), span, value_span), + }, + } }, engine_state.ctrlc.clone(), )