From 626410b2aafd60854b9b46bb577ef1eec6f714a0 Mon Sep 17 00:00:00 2001 From: Antoine Stevan <44101798+amtoine@users.noreply.github.com> Date: Wed, 22 Mar 2023 14:06:47 +0100 Subject: [PATCH] FEATURE: write better errors for `error make` and complete the doc (#8511) # Description this PR - refactors `ErrorMake::run` to avoid duplicate branches depending on the value of `--unspanned` - completes the examples 1. show a really simple `error make` call, without any command definition 2. show a complete error format with all possible fields 3. the command definition but with indentation and slightly better description - adds results to the first two examples - gives meaningful error messages for all known "bad" error formats, using the span of the error format or the span of `$format.label` to better explain why the format is bad # User-Facing Changes users have now the following help ```bash Examples: Create a simple custom error > error make {msg: "my custom error message"} Create a more complex custom error > error make { msg: "my custom error message" label: { text: "my custom label text" # not mandatory unless $.label exists start: 123 # not mandatory unless $.label.end is set end: 456 # not mandatory unless $.label.start is set } } Create a custom error for a custom command that shows the span of the argument > def foo [x] { let span = (metadata $x).span; error make { msg: "this is fishy" label: { text: "fish right here" start: $span.start end: $span.end } } } ``` and the following error messages when the error format is bad https://asciinema.org/a/568107 :partying_face: # Tests + Formatting - :green_circle: `cargo fmt --all` - :green_circle: `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` - :red_circle: `cargo test --workspace` => the tests do not pass but they do not pass on latest `main` either => i should `cargo clean`, but that's an expensive operation on my machine... # After Submitting the documentation would have to be regenerated over on the website --- .../src/core_commands/error_make.rs | 116 +++++++++++++----- 1 file changed, 86 insertions(+), 30 deletions(-) diff --git a/crates/nu-cmd-lang/src/core_commands/error_make.rs b/crates/nu-cmd-lang/src/core_commands/error_make.rs index b26a2a1b7..25efbecf4 100644 --- a/crates/nu-cmd-lang/src/core_commands/error_make.rs +++ b/crates/nu-cmd-lang/src/core_commands/error_make.rs @@ -44,43 +44,66 @@ impl Command for ErrorMake { let arg: Value = call.req(engine_state, stack, 0)?; let unspanned = call.has_flag("unspanned"); - if unspanned { - Err(make_error(&arg, None).unwrap_or_else(|| { - ShellError::GenericError( - "Creating error value not supported.".into(), - "unsupported error format".into(), - Some(span), - None, - Vec::new(), - ) - })) - } else { - Err(make_error(&arg, Some(span)).unwrap_or_else(|| { - ShellError::GenericError( - "Creating error value not supported.".into(), - "unsupported error format".into(), - Some(span), - None, - Vec::new(), - ) - })) - } + let throw_error = if unspanned { None } else { Some(span) }; + Err(make_error(&arg, throw_error).unwrap_or_else(|| { + ShellError::GenericError( + "Creating error value not supported.".into(), + "unsupported error format".into(), + Some(span), + None, + Vec::new(), + ) + })) } fn examples(&self) -> Vec { vec![ Example { - description: "Create a custom error for a custom command", - example: r#"def foo [x] { - let span = (metadata $x).span; - error make {msg: "this is fishy", label: {text: "fish right here", start: $span.start, end: $span.end } } - }"#, - result: None, + description: "Create a simple custom error", + example: r#"error make {msg: "my custom error message"}"#, + result: Some(Value::Error { + error: Box::new(ShellError::GenericError( + "my custom error message".to_string(), + "".to_string(), + None, + None, + Vec::new(), + )), + }), }, Example { - description: "Create a simple custom error for a custom command", + description: "Create a more complex custom error", + example: r#"error make { + msg: "my custom error message" + label: { + text: "my custom label text" # not mandatory unless $.label exists + start: 123 # not mandatory unless $.label.end is set + end: 456 # not mandatory unless $.label.start is set + } + }"#, + result: Some(Value::Error { + error: Box::new(ShellError::GenericError( + "my custom error message".to_string(), + "my custom label text".to_string(), + Some(Span::new(123, 456)), + None, + Vec::new(), + )), + }), + }, + Example { + description: + "Create a custom error for a custom command that shows the span of the argument", example: r#"def foo [x] { - error make {msg: "this is fishy"} + let span = (metadata $x).span; + error make { + msg: "this is fishy" + label: { + text: "fish right here" + start: $span.start + end: $span.end + } + } }"#, result: None, }, @@ -89,7 +112,7 @@ impl Command for ErrorMake { } fn make_error(value: &Value, throw_span: Option) -> Option { - if let Value::Record { .. } = &value { + if let Value::Record { span, .. } = &value { let msg = value.get_data_by_key("msg"); let label = value.get_data_by_key("label"); @@ -99,6 +122,11 @@ fn make_error(value: &Value, throw_span: Option) -> Option { let label_end = label.get_data_by_key("end"); let label_text = label.get_data_by_key("text"); + let label_span = match label.span() { + Ok(lspan) => Some(lspan), + Err(_) => None, + }; + match (label_start, label_end, label_text) { ( Some(Value::Int { val: start, .. }), @@ -126,6 +154,27 @@ fn make_error(value: &Value, throw_span: Option) -> Option { None, Vec::new(), )), + (_, _, None) => Some(ShellError::GenericError( + "Unable to parse error format.".into(), + "missing required member `$.label.text`".into(), + label_span, + None, + Vec::new(), + )), + (Some(Value::Int { .. }), None, _) => Some(ShellError::GenericError( + "Unable to parse error format.".into(), + "missing required member `$.label.end`".into(), + label_span, + Some("required because `$.label.start` is set".to_string()), + Vec::new(), + )), + (None, Some(Value::Int { .. }), _) => Some(ShellError::GenericError( + "Unable to parse error format.".into(), + "missing required member `$.label.start`".into(), + label_span, + Some("required because `$.label.end` is set".to_string()), + Vec::new(), + )), _ => None, } } @@ -136,6 +185,13 @@ fn make_error(value: &Value, throw_span: Option) -> Option { None, Vec::new(), )), + (None, _) => Some(ShellError::GenericError( + "Unable to parse error format.".into(), + "missing required member `$.msg`".into(), + Some(*span), + None, + Vec::new(), + )), _ => None, } } else {