From cc805f3f013aa7e3696c24fecf43f0b1922591af Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Joaqu=C3=ADn=20Tri=C3=B1anes?= <hi@joaquint.io>
Date: Sun, 27 Aug 2023 13:54:15 +0200
Subject: [PATCH] Screen reader-friendly errors (#10122)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Hopefully closes #10120

# Description

This PR adds a new config item, `error_style`. It will render errors in
a screen reader friendly mode when set to `"simple"`. This is done using
`miette`'s own `NarratableReportHandler`, which seamlessly replaces the
default one when needed.

Before:
```
Error: nu::shell::external_command

  × External command failed
   ╭─[entry #2:1:1]
 1 │ doesnt exist
   · ───┬──
   ·    ╰── executable was not found
   ╰────
  help: No such file or directory (os error 2)
```

After:
```
Error: External command failed
    Diagnostic severity: error
Begin snippet for entry #4 starting at line 1, column 1

snippet line 1: doesnt exist
    label at line 1, columns 1 to 6: executable was not found
diagnostic help: No such file or directory (os error 2)
diagnostic code: nu::shell::external_command

```

## Things to be determined

- ~Review naming. `errors.style` is not _that_ consistent with the rest
of the code. Menus use a `style` record, but table rendering mode is set
via `mode`.~ As it's a single config, we're using `error_style` for now.
- Should this kind of setting be toggable with one single parameter?
`accessibility.no_decorations` or similar, which would adjust the style
of both errors and tables accordingly.

# User-Facing Changes

No changes by default, errors will be rendered differently if
`error_style` is set to `simple`.

# 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 -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use std testing; testing run-tests --path
crates/nu-std"` to run the tests for the standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->

# After Submitting

There's a PR updating the docs over here
https://github.com/nushell/nushell.github.io/pull/1026
---
 crates/nu-protocol/src/cli_error.rs           | 34 ++++++++----
 crates/nu-protocol/src/config.rs              | 13 ++++-
 crates/nu-protocol/tests/test_config.rs       | 55 +++++++++++++++++++
 .../src/sample_config/default_config.nu       |  2 +
 4 files changed, 93 insertions(+), 11 deletions(-)

diff --git a/crates/nu-protocol/src/cli_error.rs b/crates/nu-protocol/src/cli_error.rs
index b563ff003c..a02bfd7bc3 100644
--- a/crates/nu-protocol/src/cli_error.rs
+++ b/crates/nu-protocol/src/cli_error.rs
@@ -1,5 +1,8 @@
 use crate::engine::{EngineState, StateWorkingSet};
-use miette::{LabeledSpan, MietteHandlerOpts, ReportHandler, RgbColors, Severity, SourceCode};
+use miette::{
+    LabeledSpan, MietteHandlerOpts, NarratableReportHandler, ReportHandler, RgbColors, Severity,
+    SourceCode,
+};
 use thiserror::Error;
 
 /// This error exists so that we can defer SourceCode handling. It simply
@@ -41,16 +44,27 @@ pub fn report_error_new(
 
 impl std::fmt::Debug for CliError<'_> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        let ansi_support = self.1.get_config().use_ansi_coloring;
+        let config = self.1.get_config();
 
-        let miette_handler = MietteHandlerOpts::new()
-            // For better support of terminal themes use the ANSI coloring
-            .rgb_colors(RgbColors::Never)
-            // If ansi support is disabled in the config disable the eye-candy
-            .color(ansi_support)
-            .unicode(ansi_support)
-            .terminal_links(ansi_support)
-            .build();
+        let ansi_support = &config.use_ansi_coloring;
+        let ansi_support = *ansi_support;
+
+        let error_style = &config.error_style.as_str();
+        let errors_style = *error_style;
+
+        let miette_handler: Box<dyn ReportHandler> = match errors_style {
+            "plain" => Box::new(NarratableReportHandler::new()),
+            _ => Box::new(
+                MietteHandlerOpts::new()
+                    // For better support of terminal themes use the ANSI coloring
+                    .rgb_colors(RgbColors::Never)
+                    // If ansi support is disabled in the config disable the eye-candy
+                    .color(ansi_support)
+                    .unicode(ansi_support)
+                    .terminal_links(ansi_support)
+                    .build(),
+            ),
+        };
 
         // Ignore error to prevent format! panics. This can happen if span points at some
         // inaccessible location, for example by calling `report_error()` with wrong working set.
diff --git a/crates/nu-protocol/src/config.rs b/crates/nu-protocol/src/config.rs
index 4c09062a4a..750ed949a8 100644
--- a/crates/nu-protocol/src/config.rs
+++ b/crates/nu-protocol/src/config.rs
@@ -26,7 +26,7 @@ pub struct ParsedMenu {
     pub source: Value,
 }
 
-/// Definition of a parsed menu from the config object
+/// Definition of a parsed hook from the config object
 #[derive(Serialize, Deserialize, Clone, Debug)]
 pub struct Hooks {
     pub pre_prompt: Option<Value>,
@@ -113,6 +113,7 @@ pub struct Config {
     pub cursor_shape_emacs: NuCursorShape,
     pub datetime_normal_format: Option<String>,
     pub datetime_table_format: Option<String>,
+    pub error_style: String,
 }
 
 impl Default for Config {
@@ -175,6 +176,8 @@ impl Default for Config {
             menus: Vec::new(),
 
             keybindings: Vec::new(),
+
+            error_style: "fancy".into(),
         }
     }
 }
@@ -1322,6 +1325,14 @@ impl Value {
                             );
                         }
                     }
+                    "error_style" => {
+                        if let Ok(style) = value.as_string() {
+                            config.error_style = style;
+                        } else {
+                            invalid!(Some(span), "should be a string");
+                            vals[index] = Value::string(config.error_style.clone(), span);
+                        }
+                    }
                     // Catch all
                     x => {
                         invalid_key!(
diff --git a/crates/nu-protocol/tests/test_config.rs b/crates/nu-protocol/tests/test_config.rs
index 4efe46fb67..c545223d78 100644
--- a/crates/nu-protocol/tests/test_config.rs
+++ b/crates/nu-protocol/tests/test_config.rs
@@ -49,3 +49,58 @@ fn filesize_format_auto_metric_false() {
     let actual = nu!(nu_repl_code(code));
     assert_eq!(actual.out, r#"["1.9 MiB", "1.9 GiB", "1.8 TiB"]"#);
 }
+
+#[test]
+fn fancy_default_errors() {
+    let actual = nu!(nu_repl_code(&[
+        r#"def force_error [x] {
+        let span = (metadata $x).span;
+        error make {
+            msg: "oh no!"
+            label: {
+                text: "here's the error"
+                start: $span.start
+                end: $span.end
+            }
+        }
+    }"#,
+        r#"force_error "My error""#
+    ]));
+
+    assert_eq!(
+        actual.err,
+        "Error:   \u{1b}[31m×\u{1b}[0m oh no!\n   ╭─[\u{1b}[36;1;4mline1\u{1b}[0m:1:1]\n \u{1b}[2m1\u{1b}[0m │ force_error \"My error\"\n   · \u{1b}[35;1m            ─────┬────\u{1b}[0m\n   ·                  \u{1b}[35;1m╰── \u{1b}[35;1mhere's the error\u{1b}[0m\u{1b}[0m\n   ╰────\n\n\n"
+    );
+}
+
+#[test]
+fn narratable_errors() {
+    let actual = nu!(nu_repl_code(&[
+        r#"$env.config = { error_style: "plain" }"#,
+        r#"def force_error [x] {
+        let span = (metadata $x).span;
+        error make {
+            msg: "oh no!"
+            label: {
+                text: "here's the error"
+                start: $span.start
+                end: $span.end
+            }
+        }
+    }"#,
+        r#"force_error "my error""#,
+    ]));
+
+    assert_eq!(
+        actual.err,
+        r#"Error: oh no!
+    Diagnostic severity: error
+Begin snippet for line2 starting at line 1, column 1
+
+snippet line 1: force_error "my error"
+    label at line 1, columns 13 to 22: here's the error
+
+
+"#,
+    );
+}
diff --git a/crates/nu-utils/src/sample_config/default_config.nu b/crates/nu-utils/src/sample_config/default_config.nu
index 7cd8e0180c..6dd9c54d66 100644
--- a/crates/nu-utils/src/sample_config/default_config.nu
+++ b/crates/nu-utils/src/sample_config/default_config.nu
@@ -167,6 +167,8 @@ $env.config = {
         header_on_separator: false # show header text on separator/border line
     }
 
+    error_style: "fancy" # "fancy" or "plain" for screen reader-friendly error messages
+
     # datetime_format determines what a datetime rendered in the shell would look like.
     # Behavior without this configuration point will be to "humanize" the datetime display,
     # showing something like "a day ago."