Repository: zkat/miette
Pull request #321 (2023-11-14 23:29:44)

Expose additional `textwrap` options
Hi!

Thanks for the project, it's really nice.

I'm running into problems when my report has a URL in it — it's wrapping to the next line on `/` boundaries. This is both a cosmetic problem and a functional one as the link cannot be clicked.

```rust
fn main() {
    let url = "https://files.pythonhosted.org/packages/bd/24/11c3ea5a7e866bf2d97f0501d0b4b1c9bbeade102bb4b588f0d2919a5212/Werkzeug-2.0.1-py3-none-any.whl";
    let report = miette::Report::msg(format!("There was a failure with {url}")).context("Oh no");

    eprint!("{report:?}");
}
```
```
  × Oh no
  ╰─▶ There was a failure with https://files.pythonhosted.org/packages/
      bd/24/11c3ea5a7e866bf2d97f0501d0b4b1c9bbeade102bb4b588f0d2919a5212/Werkzeug-2.0.1-py3-none-any.whl
```

After some digging, I discovered this behavior is due to defaults in `textwrap` and that there was not a way to change them without reimplementing the default graphical handler which seemed relatively unpleasant to maintain. Instead, I added three new options to `MietteHandlerOpts` that are passed through to `textwrap`:

- `break_words`: to disable breaking of long words when wrapping text
- `word_separator`: to allow customization of what is considered a word when wrapping text
- `word_splitter`: to allow customization of splitting words when wrapping text

 Then, the miette handler can then be configured as follows:

```rust
miette::set_hook(Box::new(|_| {
    Box::new(
        miette::MietteHandlerOpts::new()
            .break_words(false)
            .word_separator(textwrap::WordSeparator::AsciiSpace)
            .word_splitter(textwrap::WordSplitter::NoHyphenation)
            .build(),
    )
}))?;
```

```
  × Oh no
  ╰─▶ There was a failure with
      https://files.pythonhosted.org/packages/bd/24/11c3ea5a7e866bf2d97f0501d0b4b1c9bbeade102bb4b588f0d2919a5212/Werkzeug-2.0.1-py3-none-any.whl
```

I also considered a `text_wrap_options` setting to just wrap all of the supported text wrap options. We'd still need to take `width` separately and this seemed a bit heavier so I didn't implement it here. However, it seems marginally more maintainable than adding individual settings for each text wrap option people need.

I was surprised not to see any other complaints about the default wrapping behavior — let me know if I missed anything. If you'd prefer to discuss this in an issue first, just let me know.

~I did not add test cases yet because I'm a little uncertain of the intent of your test setup. I'm happy to do so though :)~

Related:

- `rustfmt` relaxed their line wrapping to avoid breaking URLs (https://github.com/rust-lang/rustfmt/pull/911)
- https://github.com/mgeisler/textwrap/issues/384


zkat: I wonder whether it would be worthwhile to set some nicer defaults here, too...
zanieb: Yeah I wonder that as well. I'd expect this to be the default behavior, generally. It seems sensible to add the options separately from making the breaking changes to the defaults though.

Also, I added one long test, it'd be nice to use `test_case` or something for parametrization or I can split them into separate tests for readability. Any feedback welcome!
zkat: Let's just go with this for now and see how it's received :)

Patch:
diff --git a/src/handler.rs b/src/handler.rs
index e983a55..e32f3ef 100644
--- a/src/handler.rs
+++ b/src/handler.rs
@@ -55,6 +55,9 @@ pub struct MietteHandlerOpts {
     pub(crate) context_lines: Option<usize>,
     pub(crate) tab_width: Option<usize>,
     pub(crate) with_cause_chain: Option<bool>,
+    pub(crate) break_words: Option<bool>,
+    pub(crate) word_separator: Option<textwrap::WordSeparator>,
+    pub(crate) word_splitter: Option<textwrap::WordSplitter>,
 }
 
 impl MietteHandlerOpts {
@@ -86,6 +89,27 @@ impl MietteHandlerOpts {
         self
     }
 
+    /// If true, long words can be broken when wrapping.
+    ///
+    /// If false, long words will not be broken when they exceed the width.
+    ///
+    /// Defaults to true.
+    pub fn break_words(mut self, break_words: bool) -> Self {
+        self.break_words = Some(break_words);
+        self
+    }
+
+    /// Sets the `textwrap::WordSeparator` to use when determining wrap points.
+    pub fn word_separator(mut self, word_separator: textwrap::WordSeparator) -> Self {
+        self.word_separator = Some(word_separator);
+        self
+    }
+
+    /// Sets the `textwrap::WordSplitter` to use when determining wrap points.
+    pub fn word_splitter(mut self, word_splitter: textwrap::WordSplitter) -> Self {
+        self.word_splitter = Some(word_splitter);
+        self
+    }
     /// Include the cause chain of the top-level error in the report.
     pub fn with_cause_chain(mut self) -> Self {
         self.with_cause_chain = Some(true);
@@ -233,6 +257,16 @@ impl MietteHandlerOpts {
             if let Some(w) = self.tab_width {
                 handler = handler.tab_width(w);
             }
+            if let Some(b) = self.break_words {
+                handler = handler.with_break_words(b)
+            }
+            if let Some(s) = self.word_separator {
+                handler = handler.with_word_separator(s)
+            }
+            if let Some(s) = self.word_splitter {
+                handler = handler.with_word_splitter(s)
+            }
+
             MietteHandler {
                 inner: Box::new(handler),
             }
diff --git a/src/handlers/graphical.rs b/src/handlers/graphical.rs
index 8cec88b..35e9c79 100644
--- a/src/handlers/graphical.rs
+++ b/src/handlers/graphical.rs
@@ -30,6 +30,9 @@ pub struct GraphicalReportHandler {
     pub(crate) context_lines: usize,
     pub(crate) tab_width: usize,
     pub(crate) with_cause_chain: bool,
+    pub(crate) break_words: bool,
+    pub(crate) word_separator: Option<textwrap::WordSeparator>,
+    pub(crate) word_splitter: Option<textwrap::WordSplitter>,
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -51,6 +54,9 @@ impl GraphicalReportHandler {
             context_lines: 1,
             tab_width: 4,
             with_cause_chain: true,
+            break_words: true,
+            word_separator: None,
+            word_splitter: None,
         }
     }
 
@@ -64,6 +70,9 @@ impl GraphicalReportHandler {
             context_lines: 1,
             tab_width: 4,
             with_cause_chain: true,
+            break_words: true,
+            word_separator: None,
+            word_splitter: None,
         }
     }
 
@@ -122,6 +131,24 @@ impl GraphicalReportHandler {
         self
     }
 
+    /// Enables or disables breaking of words during wrapping.
+    pub fn with_break_words(mut self, break_words: bool) -> Self {
+        self.break_words = break_words;
+        self
+    }
+
+    /// Sets the word separator to use when wrapping.
+    pub fn with_word_separator(mut self, word_separator: textwrap::WordSeparator) -> Self {
+        self.word_separator = Some(word_separator);
+        self
+    }
+
+    /// Sets the word splitter to usewhen wrapping.
+    pub fn with_word_splitter(mut self, word_splitter: textwrap::WordSplitter) -> Self {
+        self.word_splitter = Some(word_splitter);
+        self
+    }
+
     /// Sets the 'global' footer for this handler.
     pub fn with_footer(mut self, footer: String) -> Self {
         self.footer = Some(footer);
@@ -159,9 +186,17 @@ impl GraphicalReportHandler {
         if let Some(footer) = &self.footer {
             writeln!(f)?;
             let width = self.termwidth.saturating_sub(4);
-            let opts = textwrap::Options::new(width)
+            let mut opts = textwrap::Options::new(width)
                 .initial_indent("  ")
-                .subsequent_indent("  ");
+                .subsequent_indent("  ")
+                .break_words(self.break_words);
+            if let Some(word_separator) = self.word_separator {
+                opts = opts.word_separator(word_separator);
+            }
+            if let Some(word_splitter) = self.word_splitter.clone() {
+                opts = opts.word_splitter(word_splitter);
+            }
+
             writeln!(f, "{}", textwrap::fill(footer, opts))?;
         }
         Ok(())
@@ -212,9 +247,16 @@ impl GraphicalReportHandler {
         let initial_indent = format!("  {} ", severity_icon.style(severity_style));
         let rest_indent = format!("  {} ", self.theme.characters.vbar.style(severity_style));
         let width = self.termwidth.saturating_sub(2);
-        let opts = textwrap::Options::new(width)
+        let mut opts = textwrap::Options::new(width)
             .initial_indent(&initial_indent)
-            .subsequent_indent(&rest_indent);
+            .subsequent_indent(&rest_indent)
+            .break_words(self.break_words);
+        if let Some(word_separator) = self.word_separator {
+            opts = opts.word_separator(word_separator);
+        }
+        if let Some(word_splitter) = self.word_splitter.clone() {
+            opts = opts.word_splitter(word_splitter);
+        }
 
         writeln!(f, "{}", textwrap::fill(&diagnostic.to_string(), opts))?;
 
@@ -251,9 +293,17 @@ impl GraphicalReportHandler {
                 )
                 .style(severity_style)
                 .to_string();
-                let opts = textwrap::Options::new(width)
+                let mut opts = textwrap::Options::new(width)
                     .initial_indent(&initial_indent)
-                    .subsequent_indent(&rest_indent);
+                    .subsequent_indent(&rest_indent)
+                    .break_words(self.break_words);
+                if let Some(word_separator) = self.word_separator {
+                    opts = opts.word_separator(word_separator);
+                }
+                if let Some(word_splitter) = self.word_splitter.clone() {
+                    opts = opts.word_splitter(word_splitter);
+                }
+
                 match error {
                     ErrorKind::Diagnostic(diag) => {
                         let mut inner = String::new();
@@ -280,9 +330,17 @@ impl GraphicalReportHandler {
         if let Some(help) = diagnostic.help() {
             let width = self.termwidth.saturating_sub(4);
             let initial_indent = "  help: ".style(self.theme.styles.help).to_string();
-            let opts = textwrap::Options::new(width)
+            let mut opts = textwrap::Options::new(width)
                 .initial_indent(&initial_indent)
-                .subsequent_indent("        ");
+                .subsequent_indent("        ")
+                .break_words(self.break_words);
+            if let Some(word_separator) = self.word_separator {
+                opts = opts.word_separator(word_separator);
+            }
+            if let Some(word_splitter) = self.word_splitter.clone() {
+                opts = opts.word_splitter(word_splitter);
+            }
+
             writeln!(f, "{}", textwrap::fill(&help.to_string(), opts))?;
         }
         Ok(())
diff --git a/src/lib.rs b/src/lib.rs
index 3cb021b..08de732 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -593,6 +593,7 @@
 //!             .unicode(false)
 //!             .context_lines(3)
 //!             .tab_width(4)
+//!             .break_words(true)
 //!             .build(),
 //!     )
 //! }))


Test patch:
diff --git a/tests/graphical.rs b/tests/graphical.rs
index d24879a..536efcd 100644
--- a/tests/graphical.rs
+++ b/tests/graphical.rs
@@ -34,6 +34,190 @@ fn fmt_report(diag: Report) -> String {
     out
 }
 
+fn fmt_report_with_settings(
+    diag: Report,
+    with_settings: fn(GraphicalReportHandler) -> GraphicalReportHandler,
+) -> String {
+    let mut out = String::new();
+
+    let handler = with_settings(GraphicalReportHandler::new_themed(
+        GraphicalTheme::unicode_nocolor(),
+    ));
+
+    handler.render_report(&mut out, diag.as_ref()).unwrap();
+
+    println!("Error:\n```\n{}\n```", out);
+
+    out
+}
+
+#[test]
+fn word_wrap_options() -> Result<(), MietteError> {
+    // By default, a long word should not break
+    let out =
+        fmt_report_with_settings(Report::msg("abcdefghijklmnopqrstuvwxyz"), |handler| handler);
+
+    let expected = "  × abcdefghijklmnopqrstuvwxyz\n".to_string();
+    assert_eq!(expected, out);
+
+    // A long word can break with a smaller width
+    let out = fmt_report_with_settings(Report::msg("abcdefghijklmnopqrstuvwxyz"), |handler| {
+        handler.with_width(10)
+    });
+    let expected = r#"  × abcd
+  │ efgh
+  │ ijkl
+  │ mnop
+  │ qrst
+  │ uvwx
+  │ yz
+"#
+    .to_string();
+    assert_eq!(expected, out);
+
+    // Unless, word breaking is disabled
+    let out = fmt_report_with_settings(Report::msg("abcdefghijklmnopqrstuvwxyz"), |handler| {
+        handler.with_width(10).with_break_words(false)
+    });
+    let expected = "  × abcdefghijklmnopqrstuvwxyz\n".to_string();
+    assert_eq!(expected, out);
+
+    // Breaks should start at the boundary of each word if possible
+    let out = fmt_report_with_settings(
+        Report::msg("12 123 1234 12345 123456 1234567 1234567890"),
+        |handler| handler.with_width(10),
+    );
+    let expected = r#"  × 12
+  │ 123
+  │ 1234
+  │ 1234
+  │ 5
+  │ 1234
+  │ 56
+  │ 1234
+  │ 567
+  │ 1234
+  │ 5678
+  │ 90
+"#
+    .to_string();
+    assert_eq!(expected, out);
+
+    // But long words should not break if word breaking is disabled
+    let out = fmt_report_with_settings(
+        Report::msg("12 123 1234 12345 123456 1234567 1234567890"),
+        |handler| handler.with_width(10).with_break_words(false),
+    );
+    let expected = r#"  × 12
+  │ 123
+  │ 1234
+  │ 12345
+  │ 123456
+  │ 1234567
+  │ 1234567890
+"#
+    .to_string();
+    assert_eq!(expected, out);
+
+    // Unless, of course, there are hyphens
+    let out = fmt_report_with_settings(
+        Report::msg("a-b a-b-c a-b-c-d a-b-c-d-e a-b-c-d-e-f a-b-c-d-e-f-g a-b-c-d-e-f-g-h"),
+        |handler| handler.with_width(10).with_break_words(false),
+    );
+    let expected = r#"  × a-b
+  │ a-b-
+  │ c a-
+  │ b-c-
+  │ d a-
+  │ b-c-
+  │ d-e
+  │ a-b-
+  │ c-d-
+  │ e-f
+  │ a-b-
+  │ c-d-
+  │ e-f-
+  │ g a-
+  │ b-c-
+  │ d-e-
+  │ f-g-
+  │ h
+"#
+    .to_string();
+    assert_eq!(expected, out);
+
+    // Which requires an additional opt-out
+    let out = fmt_report_with_settings(
+        Report::msg("a-b a-b-c a-b-c-d a-b-c-d-e a-b-c-d-e-f a-b-c-d-e-f-g a-b-c-d-e-f-g-h"),
+        |handler| {
+            handler
+                .with_width(10)
+                .with_break_words(false)
+                .with_word_splitter(textwrap::WordSplitter::NoHyphenation)
+        },
+    );
+    let expected = r#"  × a-b
+  │ a-b-c
+  │ a-b-c-d
+  │ a-b-c-d-e
+  │ a-b-c-d-e-f
+  │ a-b-c-d-e-f-g
+  │ a-b-c-d-e-f-g-h
+"#
+    .to_string();
+    assert_eq!(expected, out);
+
+    // Or if there are _other_ unicode word boundaries
+    let out = fmt_report_with_settings(
+        Report::msg("a/b a/b/c a/b/c/d a/b/c/d/e a/b/c/d/e/f a/b/c/d/e/f/g a/b/c/d/e/f/g/h"),
+        |handler| handler.with_width(10).with_break_words(false),
+    );
+    let expected = r#"  × a/b
+  │ a/b/
+  │ c a/
+  │ b/c/
+  │ d a/
+  │ b/c/
+  │ d/e
+  │ a/b/
+  │ c/d/
+  │ e/f
+  │ a/b/
+  │ c/d/
+  │ e/f/
+  │ g a/
+  │ b/c/
+  │ d/e/
+  │ f/g/
+  │ h
+"#
+    .to_string();
+    assert_eq!(expected, out);
+
+    // Such things require you to opt-in to only breaking on ASCII whitespace
+    let out = fmt_report_with_settings(
+        Report::msg("a/b a/b/c a/b/c/d a/b/c/d/e a/b/c/d/e/f a/b/c/d/e/f/g a/b/c/d/e/f/g/h"),
+        |handler| {
+            handler
+                .with_width(10)
+                .with_break_words(false)
+                .with_word_separator(textwrap::WordSeparator::AsciiSpace)
+        },
+    );
+    let expected = r#"  × a/b
+  │ a/b/c
+  │ a/b/c/d
+  │ a/b/c/d/e
+  │ a/b/c/d/e/f
+  │ a/b/c/d/e/f/g
+  │ a/b/c/d/e/f/g/h
+"#
+    .to_string();
+    assert_eq!(expected, out);
+
+    Ok(())
+}
+
 #[test]
 fn empty_source() -> Result<(), MietteError> {
     #[derive(Debug, Diagnostic, Error)]