Conversation
We're constructing `textwrap::Options` in a lot of places, leading to a lot of duplicated code. Let's simplify it a bit.
| } | ||
| Ok((context_data, lines)) | ||
| } | ||
|
|
There was a problem hiding this comment.
This seems like it might be a pessimization? It's cloning the word separate and splitter regardless of whether they changed., and it saves very few lines.
I agree there's probably a bit more elegant of a pattern in here somewhere, but personally (though my opinion doesn't matter, I'm not the creator nor maintainer), this doesn't seem better, it just seems less direct.
There was a problem hiding this comment.
Is that correct? I think these are all cloned at the use-sites in the code before this change.
There was a problem hiding this comment.
Only the splitter was cloned before, I think? now both are.
There was a problem hiding this comment.
And initial_indent and subsequent_indent are called twice in some cases after this change.
There was a problem hiding this comment.
I almost wonder if a better approach would be to have this method return a textwrap::Options with the width, separator, splitter, and break_words populated, and the rest left to the call site. That'd fix the multiple initial_indent and subsequent_indent call problems.
There was a problem hiding this comment.
Or maybe have GraphicalReportHandler keep an instance of textwrap::Options within itself, rather than keeping the separator and splitter and break words separately? idk, just brainstorming.
2 participants
We're constructing
textwrap::Optionsin a lot of places, leading to a lot of duplicated code. Let's simplify it a bit.This PR only tidies the code; it was written while preparing #452, which changes the default values for some of these settings.