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

Stabilize binop_separator. #4144

Merged
merged 1 commit into from
May 8, 2020
Merged

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Apr 30, 2020

Servo has used this since forever, and it'd be useful to be able to use
rustfmt stable there so that we can use the same rustfmt version in
both Firefox and Servo.

Feel free to close this if there's any reason it shouldn't be done.

Closes #3368

Servo has used this since forever, and it'd be useful to be able to use
rustfmt stable there so that we can use the same rustfmt version in
both Firefox and Servo.

Feel free to close this if there's any reason it shouldn't be done.

Closes rust-lang#3368
@calebcartwright
Copy link
Member

I think we'll need to resolve #3208 before stabilizing this.

it'd be useful to be able to use
rustfmt stable there so that we can use the same rustfmt version in
both Firefox and Servo

Technically you already can use binop_separator on stable today if you're willing to set it via cli arg (#4081)

cargo +stable fmt -- --config binop_separator=back 

Note there's a good chance that may change when we release rustfmt 2.0 though.

@emilio
Copy link
Contributor Author

emilio commented May 1, 2020

I think we'll need to resolve #3208 before stabilizing this.

Hmm? I'm not sure I agree with the expectation, fwiw. Current output looks fine to me but.. :)

@calebcartwright
Copy link
Member

We can't stabilize a config option that has open issues (https://github.com/rust-lang/rustfmt/blob/master/Processes.md#conditions).

Regardless of how #3208 is resolved, (even if that does indeed turn out to be a closed/wont-fix) we would need to get that one closed before merging this.

@emilio
Copy link
Contributor Author

emilio commented May 1, 2020

Sure, that's fine. I had searched for open issues fwiw, that one just didn't show up because the name of the option wasn't mentioned :)

Well then this is fine to wait or what not. Though given it's been open since 2018 and without any progress, or other complaints, maybe it's fine to wontfix it. Anyhow not my call, so let me know if you want me to close this or leave this open, etc.

Thanks :)

@calebcartwright
Copy link
Member

Sure, that's fine. I had searched for open issues fwiw, that one just didn't show up because the name of the option wasn't mentioned :)

No worries! I feel I'd seen this one not too long ago so it was in the back of my brain already.

Though given it's been open since 2018 and without any progress, or other complaints, maybe it's fine to wontfix it

In fairness there's probably quite a few unstable config options that fall into that bucket, but that's mostly because there hasn't been enough bandwidth to dig into the unstable opts. We've been mostly focused on getting rustfmt 2.0 out (and dealing with upstream rustc changes), and were generally planning to revisit the unstable opts after the 2.0 release.

Anyhow not my call, so let me know if you want me to close this or leave this open, etc.

I'm planning to leave it open for now. Based on the below threads from the style guide, it seems the indent shown in #3208 is aligned with the style guide so we may be able to close that and merge this in time for the 2.0 release. Though if #3208 ends up needing work to resolve then will probably close this

rust-lang/style-team#18
rust-lang/style-team#115

@topecongiro
Copy link
Contributor

@emilio @calebcartwright My apologies for the delay in confirming #3208. The issue doesn't seem to be a problem; rustfmt adds an extra indentation if binary expressions span to multiple lines. I will go ahead and close the issue as working as expected.

@rakshith-ravi
Copy link

So can we stabilize this?

@calebcartwright calebcartwright merged commit c83e729 into rust-lang:master May 8, 2020
@emilio emilio deleted the binop-stable branch May 8, 2020 23:46
emilio added a commit to emilio/rustfmt that referenced this pull request Sep 28, 2022
See reasoning in rust-lang#4144, which still applies. Other projects like bindgen
etc also use it by default, and this would allow them to move to stable
rust.
emilio added a commit to emilio/rustfmt that referenced this pull request Sep 28, 2022
See reasoning in rust-lang#4144, which still applies. Other projects like bindgen
etc also use it by default, and this would allow them to move to stable
rust.

Fixes rust-lang#3368 again.
emilio added a commit to emilio/rustfmt that referenced this pull request Dec 19, 2022
See reasoning in rust-lang#4144, which still applies. Other projects like bindgen
etc also use it by default, and this would allow them to move to stable
rust.

Fixes rust-lang#3368 again.
emilio added a commit to emilio/rustfmt that referenced this pull request Nov 20, 2023
See reasoning in rust-lang#4144, which still applies. Other projects like bindgen
etc also use it by default, and this would allow them to move to stable
rust.

Fixes rust-lang#3368 again.
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.

[unstable option] binop_separator
5 participants