Expose additional textwrap options#321
Conversation
| 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); | ||
| } |
There was a problem hiding this comment.
Since this is done repeatedly, perhaps a helper method should be introduced.
|
I wonder whether it would be worthwhile to set some nicer defaults here, too... |
|
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 |
|
Let's just go with this for now and see how it's received :) |
|
@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. |
|
oh duh. I don't know why that didn't jump out at me. Maybe we can |
|
I think you already gate everywhere these changes are imported with the 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? |
|
oh. I guess we're all set, then? |
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
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.After some digging, I discovered this behavior is due to defaults in
textwrapand 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 toMietteHandlerOptsthat are passed through totextwrap:break_words: to disable breaking of long words when wrapping textword_separator: to allow customization of what is considered a word when wrapping textword_splitter: to allow customization of splitting words when wrapping textThen, the miette handler can then be configured as follows:
I also considered a
text_wrap_optionssetting to just wrap all of the supported text wrap options. We'd still need to takewidthseparately 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:
rustfmtrelaxed their line wrapping to avoid breaking URLs (Don't be so aggressive about line-breaking strings rust-lang/rustfmt#911)