Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose additional textwrap options #321

Merged
merged 5 commits into from
Nov 15, 2023
Merged

Conversation

zanieb
Copy link
Contributor

@zanieb zanieb commented Nov 14, 2023

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.

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:

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:

Comment on lines -254 to +305
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);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is done repeatedly, perhaps a helper method should be introduced.

@zkat
Copy link
Owner

zkat commented Nov 15, 2023

I wonder whether it would be worthwhile to set some nicer defaults here, too...

@zanieb
Copy link
Contributor Author

zanieb commented Nov 15, 2023

Yeah I wonder that as well. I'd expect this to be the default behavior, generally. There are some caveats for non-English languages though and the Unicode line break algorithm seems better sometimes? Regardless, it seems sensible to add the options separately from making the breaking changes to the defaults.

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
Copy link
Owner

zkat commented Nov 15, 2023

Let's just go with this for now and see how it's received :)

@zkat zkat merged commit fd77257 into zkat:main Nov 15, 2023
11 checks passed
@zanieb
Copy link
Contributor Author

zanieb commented Nov 16, 2023

@zkat One of my co-workers pointed out that this introduces a dependency on textwrap in your public API which may be undesirable since breaking changes in textwrap will require a major version release here. I think the only alternative is to introduce some wrapper types which sounds... annoying... but may be better practice.

@zkat
Copy link
Owner

zkat commented Nov 16, 2023

oh duh. I don't know why that didn't jump out at me. Maybe we can cfg them away if not(fancy)?

@zanieb
Copy link
Contributor Author

zanieb commented Nov 16, 2023

I think you already gate everywhere these changes are imported with the fancy-no-backtrace and fancy features. Might be worth a double check, but I don't think this makes textwrap required.

I don't think adding more feature flagging is helpful for avoiding exposing textwrap types as a part of your public API though, unless I'm misunderstanding?

@zkat
Copy link
Owner

zkat commented Nov 16, 2023

oh. I guess we're all set, then?

zanieb added a commit to astral-sh/uv that referenced this pull request Nov 17, 2023
Extends #424 with support for URL dependency incompatibilities.

Requires changes to `miette` to prevent URLs from being word wrapped;
accepted upstream in zkat/miette#321
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants