Skip to content
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

Update width heuristic configuration #4063

Merged
merged 5 commits into from
Apr 18, 2020

Conversation

calebcartwright
Copy link
Member

Closes #3710

The diff makes this seem bigger than it actually is, but there's really not all that much in this PR.

In addition to max_width, rustfmt has internally used 7 different max width settings for specific code constructs as part of the heuristics for determining whether to use multi-line/vertical formatting. These private settings were set based on the value provided for the public/user-facing use_small_heuristics config option. Essentially, use_small_heuristic was the public facing knob that controlled 7 private options.

That's helpful in scenarios where users want a minimal config and want to be able to simply update and manage one "width" config option (max_width), but it was problematic for users that wanted to have more granular control over those individual width settings.

With this change rustfmt should now be able to support both as all the individual width heuristic config options are now public and available for users to set, but there's also still a global heuristic setting that can be used as well.

In this PR:

  • The aggregate heuristic setting was renamed from use_small_heuristics to width_heuristics and the Default variant was renamed to Scaled
    • There's been several questions in Discord, issue/PR threads, etc. that indicate folks found those names confusing and I think this makes them a bit clearer
    • This option is still used to set values for all the granular width config, but user-provided values take precedence
  • The previously internal/private *_width config options are all now public and can be set/overriden by users.
  • rustfmt will error if a user provides a value for one of the individual *_width options that exceeds the value of max_width

@benesch
Copy link
Contributor

benesch commented Apr 7, 2020

What's the status of this PR? Not having control over line length is my single greatest frustration with rustfmt—would absolutely love to see this merged! Is there anything I can do to help?

@calebcartwright
Copy link
Member Author

would absolutely love to see this merged! Is there anything I can do to help?

Thanks for the offer @benesch! It's a fairly sizeable change, so really just needs a review from @topecongiro

Please also note that this will be part of the rustfmt 2.0 release (which does not yet have a planned date). That means that even once this is merged it will be still be some time before this change would be available for consumption except for folks building rustfmt from source.

Not having control over line length is my single greatest frustration with rustfmt

Just out of curiosity, Is that frustration specifically with the lack of control/granularity available today between max_width and use_small_heuristics? I'm glad to hear this will help but it probably won't solve everything 😄

I'm guessing this will most benefit users that like some, but not all, aspects of settings controlled by use_small_heuristics today.

@benesch
Copy link
Contributor

benesch commented Apr 12, 2020

would absolutely love to see this merged! Is there anything I can do to help?

Thanks for the offer @benesch! It's a fairly sizeable change, so really just needs a review from @topecongiro

Ah, gotcha.

Please also note that this will be part of the rustfmt 2.0 release (which does not yet have a planned date). That means that even once this is merged it will be still be some time before this change would be available for consumption except for folks building rustfmt from source.

I'm honestly probably willing to do just that in the meantime.

Not having control over line length is my single greatest frustration with rustfmt

Just out of curiosity, Is that frustration specifically with the lack of control/granularity available today between max_width and use_small_heuristics? I'm glad to hear this will help but it probably won't solve everything 😄

Yeah, it's entirely the lack of granularity. I've tried setting use_small_heuristics to Max, and, according to my admittedly subjective taste, it makes as many things worse as it makes better.

Copy link
Contributor

@topecongiro topecongiro left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. Overall this looks good to me; Just one point, I commented on error handling.

rustfmt-core/rustfmt-config/src/config_type.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@topecongiro topecongiro left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the updates!

@topecongiro topecongiro merged commit f1a44c5 into rust-lang:master Apr 18, 2020
@calebcartwright calebcartwright deleted the width-heuristics branch April 18, 2020 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate use_small_heuristics into distinct options
4 participants