Skip to content

Separate use_small_heuristics into distinct options #3710

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

Closed
mooman219 opened this issue Jul 28, 2019 · 8 comments · Fixed by #4063
Closed

Separate use_small_heuristics into distinct options #3710

mooman219 opened this issue Jul 28, 2019 · 8 comments · Fixed by #4063
Assignees
Milestone

Comments

@mooman219
Copy link

mooman219 commented Jul 28, 2019

Feature request

Small heuristics has a mix of formatting cases that I like and dislike. Unfortunately, I can't have the mix of formatting options I want since it's an all or nothing sort of situation with use_small_heuristics. This issue is a feature request to separate out those formatting options into distinct options.

For backwards compatibility, I imagine it would be reasonable to annotate them as "overridden if use_small_heuristics is not 'off'"

Example situation

It's a small heuristic feature to format structural enums on the same line:

 match message {
     InputMessage::CloseRequested => is_active = false,
     InputMessage::KeyPressed(key) => match key {
         KeyboardButton::Escape => is_active = false,
         _ => {}
     },
+    InputMessage::CursorPressed { .. } => {
-    InputMessage::CursorPressed {
-        ..
-    } => {
         // Code
     }
+    InputMessage::CursorMoved { pos, .. } => {
-    InputMessage::CursorMoved {
-        pos,
-        ..
-    } => {
         // Code
     }
     _ => {}
 }

But if I disagree with some of the other small heuristics, I can't have this formatting option at all if I chose to set the configuration option to "off".

@topecongiro topecongiro added this to the 2.0.0 milestone Aug 13, 2019
@calebcartwright
Copy link
Member

I believe that the width limits internally set based on the value of use_small_heuristics config option were originally exposed as distinct options, and the consolidation to use_small_heuristics was done intentionally in an effort to reduce config options.

Some related issues:

Is the plan for this one to re-introduce the more granular width config options (struct_lit_width, struct_variant_width, etc.), and if so, what about use_small_heuristics, max_width, and the relationship between them all?

@topecongiro
Copy link
Contributor

@calebcartwright Thank you for picking this up.

Is the plan for this one to re-introduce the more granular width config options (struct_lit_width, struct_variant_width, etc.)

Yes.

what about use_small_heuristics, max_width, and the relationship between them all?

The value of max_width must be larger than that of each config option. And each config options should precede the values set by small heuristics.

@calebcartwright
Copy link
Member

Thanks @topecongiro. Just to validate my understanding, are the below scenarios & behaviors correct?

  1. rustfmt should emit a config error if an individual width config exceeds max width. For example if array_width is set to 120 and max_width is 100
  2. Any explicit individual width config option takes precedence over the derived values. For example, with the below config, rustfmt would use the fn_call_width of 80 even though the calculated small heuristic value for fn_call_width would be 60 based on the heuristics calc
use_small_heuristics = "Default"
max_width = 100
fn_call_width = 80
  1. Same as the second bullet above, but also when use_small_heuristics is Max or Off

@topecongiro
Copy link
Contributor

@calebcartwright Yes to every question 👍

I almost want to remove the use_small_heuristics option entirely, as it feels like a design mistake.

@calebcartwright
Copy link
Member

I almost want to remove the use_small_heuristics option entirely, as it feels like a design mistake.

👍 I definitely feel like there's an opportunity here, as use_small_heuristics has been a source of questions and confusion (both in various threads on GitHub, Discord, etc.).

My sense is that there's two separate use cases that are tricky to support simultaneously which is why there's complexity and some ambiguity around use_small_heuristics

  1. Some users want fine grained control over the underlying width limits for the various code types (chain_width, struct_lit_width), and/or they need insight into those values to minimize confusion caused by the internally calculated values (Why would rustfmt change "multiple child in one line" to multiple lines? #3957 as another example)
  2. Some users want to be able to simply leverage max_width and not have to deal with 5+ other width config values (i.e. users that want to use the max_width value for everything, users that want to use a non-default max_width value and have the more granular width controls scaled accordingly, etc.)

If we remove use_small_heuristics, that'd require some portion of the users in that second use case to have to explicitly set each of the new width config valeus (fn_call_width, attr_fn_like_width, etc.) as we'd have to pick one of those heuristic modes (currently Default, Off, Max) as the default.

Alternatively, if we keep use_small_heuristics then we could review/update the config name, values, docs, etc. to try to make things more clear.

I can start working on making the more granular configs public, but let me know how you'd like to proceed with the potential removal of use_small_heuristics @topecongiro

@mooman219
Copy link
Author

I'd be content with an override situation where if you're providing the fine grain widths, it overrides the defaults set with use_small_heuristics. This way you can ease your way into it. Otherwise, in the documentation, providing what the equivalent width settings to match use_small_heuristics output would be fine.

@tkaitchuck
Copy link

tkaitchuck commented Jan 10, 2020

The default value of struct_lit_width seems to be the one that is burning people the most, myself included. (18 is just really low, especially given the next lowest value is 35, and even inline if is 50.) There are certain cases where struct literals end up being used a lot, such as when providing context for errors.

If we don't want to change the default formatting, then we can either add additional options besides (Default, Off, and Max) which probably makes sense as both "Off" and "Max" are very extreme. For example "medium" and "high".

Alternatively we simply allow struct_lit_width to be overridden as a separate parameter.

@calebcartwright
Copy link
Member

Alternatively we simply allow struct_lit_width to be overridden as a separate parameter.

@tkaitchuck - All of the individual width config options are going to be made public (including struct_lit_width) to let folks control those width options to their liking, and this work is already in flight.

The only remaining question is whether we'll also keep use_small_heuristics (or something equivalent) to continue supporting the use case of being able to control/scale all of those individual width options as well.

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 a pull request may close this issue.

4 participants