Skip to content

WIP: Add an option for maximizing lines #2447

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

WIP: Add an option for maximizing lines #2447

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5c22f95
Add an option for maximizing lines
radix Feb 15, 2018
0ac5d05
Fix tests and update Configurations.md
radix Feb 15, 2018
bc3e3ad
fix formatting
radix Feb 15, 2018
01b41f4
add a new test for use_small_heuristics: maximum
radix Feb 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions rustfmt-config/src/config_type.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,12 @@ macro_rules! create_config {
($($i:ident: $ty:ty, $def:expr, $stb:expr, $( $dstring:expr ),+ );+ $(;)*) => (
#[derive(Clone)]
pub struct Config {
// For each config item, we store a bool indicating whether it has
// been accessed and the value, and a bool whether the option was
// manually initialised, or taken from the default,
// For each config item, we store:
// - a bool indicating whether it has been accessed
// - a bool indicating whether the option was manually initialised, or taken from the
// default,
// - the value of the option
// - whether this option is stable (if false, it will only work on nightly builds)
$($i: (Cell<bool>, bool, $ty, bool)),+
}

Expand Down Expand Up @@ -358,12 +361,12 @@ macro_rules! create_config {
}

fn set_heuristics(&mut self) {
if self.use_small_heuristics.2 {
let max_width = self.max_width.2;
self.set().width_heuristics(WidthHeuristics::scaled(max_width));
} else {
self.set().width_heuristics(WidthHeuristics::null());
}
let width = match self.use_small_heuristics.2 {
LineWidth::Minimum => WidthHeuristics::null(),
LineWidth::Medium => WidthHeuristics::scaled(self.max_width.2),
LineWidth::Maximum => WidthHeuristics::maximum(self.max_width.2),
};
self.set().width_heuristics(width);
}
}

Expand Down
3 changes: 1 addition & 2 deletions rustfmt-config/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.";
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :(

Copy link
Member

Choose a reason for hiding this comment

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

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.


// strings and comments
format_strings: bool, false, false, "Format string literals where necessary";
Expand Down
20 changes: 20 additions & 0 deletions rustfmt-config/src/options.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,15 @@ configuration_option_enum! { Color:
Auto,
}

configuration_option_enum! { LineWidth:
// Always format lines as short as possible (?)
Minimum,
Copy link
Member

Choose a reason for hiding this comment

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

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).

// Use width heuristics to determine whether to reformat a line
Medium,
// Format lines to be as long as possible
Maximum,
}

#[derive(Deserialize, Serialize, Clone, Debug)]
pub struct WidthHeuristics {
// Maximum width of the args of a function call before falling back
Expand Down Expand Up @@ -235,6 +244,17 @@ impl WidthHeuristics {
single_line_if_else_max_width: (50.0 * max_width_ratio).round() as usize,
}
}

pub fn maximum(max_width: usize) -> WidthHeuristics {
WidthHeuristics {
fn_call_width: max_width,
struct_lit_width: max_width,
struct_variant_width: max_width,
array_width: max_width,
chain_width: max_width,
single_line_if_else_max_width: max_width,
Copy link
Member

Choose a reason for hiding this comment

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

I would use usize::max_value() rather than max_width means one less arg, plus no ordering constraint on initialisation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fwiw, the scaled() constructor also takes max_width, but I can still change this np.

Copy link
Member

Choose a reason for hiding this comment

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

scaled needs it though since it uses the max width as input (we could probably use max_value there for the 100% ones).

}
}
}

impl ::std::str::FromStr for WidthHeuristics {
Expand Down