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

Expose chain_width et al in 1.4 #4782

Merged

Conversation

ctz
Copy link
Contributor

@ctz ctz commented Apr 2, 2021

This is a rebasing of #4063 onto rustfmt-1.4.37 (but let me know if that's not the right target!) with a number of edits.

I've been careful to avoid the replacement of use_small_heuristics with width_heuristics on the original PR, as that'd be a breaking change. But I have kept the improved documentation introduced there.

Finally, I added some tests that cover the behaviour rustls is relying on.

This is for #4769.

@ctz
Copy link
Contributor Author

ctz commented Apr 2, 2021

This passes cargo test for me locally; I think some of the breakage here will be fixed in #4775

@calebcartwright
Copy link
Member

This passes cargo test for me locally; I think some of the breakage here will be fixed in #4775

Indeed! Please rebase whenever you get a chacne

@ctz ctz force-pushed the jbp-expose-chain-width-1.4 branch from 061ea68 to 9d8a2e0 Compare April 5, 2021 07:22
@ctz
Copy link
Contributor Author

ctz commented Apr 5, 2021

Rebased, thanks. CI is looking healthier, with the same results as for 64ef03b

@calebcartwright
Copy link
Member

I've been careful to avoid the replacement of use_small_heuristics with width_heuristics on the original PR, as that'd be a breaking change.

Quite right! I do still want to get away from a using a binary-sounding name for a multi-variant option. I think we could probably do a soft deprecation for this like we've done for other options where we keep the old option but add a new one with the name that we want. Then when we detect the usage of the "old" option just print a warning and automap to the corresponding variant in the new option.

I don't think we necessarily need to do that here (though if you're interested in doing so please feel free!), but do be aware that it's probably coming.

Also for awareness, had to rush v1.4.37 out to deal with the broken toolstate on nightly, but have slated this one for the v1.4.38 release (I will adjust the target branch before merging)

@calebcartwright calebcartwright changed the base branch from rustfmt-1.4.37 to rustfmt-1.4.38 April 21, 2021 03:17
Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Thanks!

@calebcartwright calebcartwright merged commit 58157bb into rust-lang:rustfmt-1.4.38 Apr 22, 2021
@djc
Copy link
Contributor

djc commented Sep 6, 2021

@calebcartwright I'm trying to figure out if/when this will hit stable Rust. How do I do that? You merged it into a 1.4.38 branch, but it doesn't seem like that has been released (but then even 1.4.37 does not list as released in Releases, even as my local stable rustfmt reports being 1.4.37). Will we get 1.4.38 with Rust 1.55?

@calebcartwright
Copy link
Member

This option has been available on stable since 1.53. Are you having any issues using it/seeing any warnings about an unknown config? I still haven't finished getting the docs site back in order since we swapped our default branch back (refs #4801), but the option should still be available even though it's not listed on the site.

@djc
Copy link
Contributor

djc commented Sep 7, 2021

I just misunderstood because you had initially merged to the 1.4.38 branch. Now that I've actually tried it, it seems to work fine on stable. Thanks for all your work on rustfmt!

@calebcartwright
Copy link
Member

No worries. The branching story had gotten overly complex and although things are more simple now the prior complexity can still be a source of ambiguity. Glad it's working for you!

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.

3 participants