WIP: Add an option for maximizing lines #2447
Conversation
This just sets the heuristic widths == max_width.
|
here's a great example of why I wanted this change, from my real codebase. This is what rustfmt generated previously: TransferItem {
from,
to,
item_id,
count,
} => self.change_with(GameLog::TransferItem {
from,
to,
item_id,
count,
}),
RemoveItem {
owner,
item_id,
count,
} => self.change_with(GameLog::RemoveItem {
owner,
item_id,
count,
}),and now, with my changes, it's: TransferItem { from, to, item_id, count } => {
self.change_with(GameLog::TransferItem { from, to, item_id, count })
}
RemoveItem { owner, item_id, count } => {
self.change_with(GameLog::RemoveItem { owner, item_id, count })
}which, I notice, even fits within 80 lines. |
| @@ -48,8 +48,7 @@ create_config! { | |||
| tab_spaces: usize, 4, true, "Number of spaces per tab"; | |||
| newline_style: NewlineStyle, NewlineStyle::Unix, true, "Unix or Windows line endings"; | |||
| indent_style: IndentStyle, IndentStyle::Block, false, "How do we indent expressions or items."; | |||
| use_small_heuristics: bool, true, false, "Whether to use different formatting for items and\ | |||
| expressions if they satisfy a heuristic notion of 'small'."; | |||
| use_small_heuristics: LineWidth, LineWidth::Medium, false, "The general width of lines."; | |||
There was a problem hiding this comment.
I think it is better to talk about how aggressively we put items across multiple lines, rather than 'general width of lines', which seems to invite confusion with max_width. So, rather than LineWidth::Minimum, we talk about making items as vertically formatted as possible, whereas Maximum is about making items maximally horizontal. (Or mutli-line vs single-line, if you prefer)
There was a problem hiding this comment.
This is still the biggest thing holding me back from finishing up this PR. The line length heuristics are confusing. I don't think "more horizontal" vs "more vertical" even really describes the options.
use_small_heuristics: true: (default)
- function calls: 60%
- struct literals: 18%
- struct variants: 35%
- arrays: 60%
- chains: 60%
- if/else: 50%
use_small_heuristics: false
- function calls: 100%
- struct literals: 0%
- struct variants: 0%
- arrays: 100%
- chains: 100%
- if/else: 0%
This means that enabling heuristics leads to code being sometimes more vertical and sometimes less vertical. This makes it really hard for me to describe what use_small_heuristics: false does in plain English, which also makes it hard to come up with a non-boolean name for what it does :(
There was a problem hiding this comment.
Hmm, yeah I know what you mean. The logic is that with use_small_heuristics false, we use the same formatting for small things as we do for large things, whereas when true we do things differently depending on the size - we've never tried to frame it as making lines longer or shorted, just 'better' (easier to read, prettier, etc). I guess the trouble with describing this is that even without use_small_heuristics, the set of things which is put on one line, rather than multiple lines is somewhat arbitrary.
Perhaps values like None (today's false), Default (today's true), and Max (for the new stuff) is OK. They're not very self-explanatory, but I don't think we'll do better.
| struct_variant_width: max_width, | ||
| array_width: max_width, | ||
| chain_width: max_width, | ||
| single_line_if_else_max_width: max_width, |
There was a problem hiding this comment.
I would use usize::max_value() rather than max_width means one less arg, plus no ordering constraint on initialisation.
There was a problem hiding this comment.
fwiw, the scaled() constructor also takes max_width, but I can still change this np.
There was a problem hiding this comment.
scaled needs it though since it uses the max width as input (we could probably use max_value there for the 100% ones).
| @@ -189,6 +189,15 @@ configuration_option_enum! { Color: | |||
| Auto, | |||
| } | |||
|
|
|||
| configuration_option_enum! { LineWidth: | |||
| // Always format lines as short as possible (?) | |||
| Minimum, | |||
There was a problem hiding this comment.
The null width heuristic doesn't make lines as short as possible, it tries to layout stuff in its 'default' layout without taking into consideration the width of the sub-item. I.e., it will make function calls as wide as possible, but will never put if/else on one line (since multi-line is the default).
|
Hi, just an update on this PR to state that I do still intend to finish this off, I have just been busy with other things lately. I'm closing this PR to unclutter the PR list for the maintainers, but I will re-open it once I've addressed the comments. Thanks @nrc for being so helpful with the reviews! |
Great, look forward to seeing it again, thanks! |
#Fixes #2437
This makes
use_small_heuristicsinto a tri-state option with an explicit enum.Minimum-- equivalent to olduse_small_heuristics: falseMedium-- equivalent to olduse_small_heuristics: true(default)Maximum-- new behavior: set width heuristics equal to max_width.TODO:
use_small_heuristicsobviously isn't a good name for this any moreFeedback appreciated!