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

rustfmt let-else support #1117

Merged
merged 1 commit into from
Jul 3, 2023

Conversation

calebcartwright
Copy link
Member

@calebcartwright calebcartwright commented Jun 27, 2023

Announce Rustfmt's support for let-else statements

Rendered

I have several objectives that I hope this post achieves:

  • Communicating the start of let-else support in Rustfmt
  • Attempting to preempt specious assumptions that Rustfmt has "changed" how it formats let-else statements
  • Providing context/info around the differences and relationship between the Style Guide and Rustfmt
  • Acknowledging the delay in let-else support and providing historical context on some of the relevant circumstances
  • Communicating the various efforts, both those completed and those ongoing, to improve things moving forward

Welcome any and all feedback

@joshtriplett
Copy link
Member

I think it would be appropriate for us to say something about how we don't, in the future, anticipate adding formatting for an "unformatted" construct like this, but that we're doing so this time because not doing so makes let-else completely prevent rustfmt from doing anything at all.

@calebcartwright
Copy link
Member Author

I think it would be appropriate for us to say something about how we don't, in the future, anticipate adding formatting for an "unformatted" construct like this, but that we're doing so this time because not doing so makes let-else completely prevent rustfmt from doing anything at all.

Agreed, that's a good idea. I will probably opt for softer language though, as there could be another case or two that we need to roll out and I want to avoid this blog post completely ruling out that possibility.

@joshtriplett
Copy link
Member

joshtriplett commented Jun 29, 2023 via email

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 1, 2023
…bcartwright

Update Rustfmt (add let-else support)

Adds let-else formatting support

Bit more detail in: https://github.com/rust-lang/rustfmt/blob/master/CHANGELOG.md#160-2023-07-02
Accompanying blog post: rust-lang/blog.rust-lang.org#1117

I know we're getting close to tool week, however, there's been extensive discussion and testing of the changes in this between both t-style and t-rustfmt. Our confidence level is extremely high, and even if it's only on nightly for a few days, I'd still much prefer that and being able to get this out with 1.72 vs having to push to 1.73

Closes rust-lang/rustfmt#4914
cc `@rust-lang/style` for awareness
@calebcartwright calebcartwright marked this pull request as ready for review July 1, 2023 15:30
@calebcartwright
Copy link
Member Author

On Tue, Jun 27, 2023 at 09:09:58PM -0700, Caleb Cartwright wrote: > I think it would be appropriate for us to say something about how we don't, in the future, anticipate adding formatting for an "unformatted" construct like this, but that we're doing so this time because not doing so makes let-else completely prevent rustfmt from doing anything at all. Agreed, that's a good idea. I will probably opt for softer language though, as there could be another case or two that we need to roll out and I want to avoid this blog post completely ruling out that possibility.
Sure. My goal here was more to make sure this blog post doesn't imply it's completely OK that we're churning people's (lack of) formatting here.

@joshtriplett - added some text to cover this in https://github.com/rust-lang/blog.rust-lang.org/blob/895d20f39d3f5e973e1719dc46fc945ad3e9c8bc/posts/2023-07-01-rustfmt-supports-let-else-statements.md#conclusion

@calebcartwright
Copy link
Member Author

It seems like rust-lang/rust#113225 is well on its way to getting landed, which means let-else support should be in the next nightly.

Would be good to get this approved and merged today prior to the next nightly becoming available, if possible, as both t-rustfmt and t-style seem to be 👍 with it

@WaffleLapkin
Copy link
Member

rust-lang/rust#113225 is indeed merged, can we proceed here?

@calebcartwright
Copy link
Member Author

rust-lang/rust#113225 is indeed merged, can we proceed here?

I can and will merge if I get an approval, though IIRC that approval needs to come from someone that's part of the leads team based on how branch protections are configured in this repo.

@Manishearth or @ehuss would either of you be willing to 👍 since this is largely a dev tools sub team post?

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

looks great!

@Manishearth
Copy link
Member

(can merge, holding off on hitting the button since Caleb indicated he can do it himself and i'm happy to leave the timeline up to him)

@calebcartwright
Copy link
Member Author

Thanks Manish! I'd like to go ahead and get this out, so going to move ahead now

@calebcartwright calebcartwright merged commit a7af062 into rust-lang:master Jul 3, 2023
1 check passed
@calebcartwright calebcartwright deleted the fmt-let-else branch July 3, 2023 17:56
@WaffleLapkin
Copy link
Member

I've noticed that the date on the blog is July 1-st, is this expected? I couldn't find any guidelines, is the date supposed to be "written at" date (instead of "publish date")?

@ehuss
Copy link
Contributor

ehuss commented Jul 3, 2023

I can and will merge if I get an approval, though IIRC that approval needs to come from someone that's part of the leads team based on how branch protections are configured in this repo.

As for process, on something like this, I normally expect someone from the team the post is about to respond with some form of approval (in this case, someone on the rustfmt or style teams). I didn't see that here, so I can't know if the post represents what the teams want to say.

If a team is sufficiently bottlenecked where that isn't possible (or only has one active person), then proactively reaching out to a lead from the team above you can probably get things merged a little more quickly.

@Manishearth
Copy link
Member

@ehuss Caleb is rustfmt lead and Josh is on the style team, so I think this is fine here.

@ehuss
Copy link
Contributor

ehuss commented Jul 4, 2023

Yes, but there never was an approval from Josh, or any comment from the other member of rustfmt. I'm not saying that's not fine (or that I don't trust Caleb, I think this post was very well written), I just think in that situation to ping a specific person to merge explaining why will get a faster result. Otherwise, from my observations, the PR will likely sit waiting.

@Manishearth
Copy link
Member

Ah, fair, I think they were looking for approval from me as the devtools lead for that, since for the smaller devtools teams this stuff tends to bubble up to me. But yeah, makes sense.

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 this pull request may close these issues.

5 participants