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

Add "type_separator" option to control placement of "+" in types #5004

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ulyssa
Copy link
Contributor

@ulyssa ulyssa commented Sep 25, 2021

This adds a new type_separator option, similar to the binop_separator option, as requested in #4979. Tests for it are included in tests/{source,target}/type-separator-back/. While trying to come up with appropriate tests that included comments, I ran into #2055. I've made the necessary changes to fix that, and have included tests for it in tests/{source,target}/issue-2055.rs.

One thing I'm unsure of, and wanted to ask about: are things that are gated under Version::Two okay to change? If you look at tests/target/type-separator-back/impl-v2.rs, you can see that the behaviour of the impl keyword currently doesn't indent the lines following the first. This doesn't match the indentation behaviour of dyn and others, but I didn't want to change it without checking first since doing so would break the impl keyword test in tests/target/issue-3701/two.rs.

@calebcartwright
Copy link
Member

This adds a new type_separator option, similar to the binop_separator option, as requested in #4979. Tests for it are included in tests/{source,target}/type-separator-back/. While trying to come up with appropriate tests that included comments, I ran into #2055. I've made the necessary changes to fix that, and have included tests for it in tests/{source,target}/issue-2055.rs.

Thank your another PR!

I think it would probably be best to split these into two separate PRs given the size and dependency. Also worth noting that there is a separate branch in this repo that contains what is at this point a significantly different version of the codebase, which was intended to be used for a major/breaking v2 release of rustfmt (/rustfmt-2.0.0-rc.2 - https://github.com/rust-lang/rustfmt/tree/rustfmt-2.0.0-rc.2)

That breaking release is unlikely to happen any time soon, if ever, but it's relevant here because there were some changes/fixes applied to that branch which may be usable, or at least worth referencing. There's also some directly related changes that were proposed (at least in part) around the issues discussed in #2055 that I wanted to make sure you were aware of as well, refs #4666.

Don't feel obligated to copy/constrained by any other such changes on the 2.x side though. I haven't had a chance to look at any of the actual changes here and no matter what we need to get the fixes/improvements into the main branch so that we can actually get them out to users, but wanted to you let you know there was some prior art.

One thing I'm unsure of, and wanted to ask about: are things that are gated under Version::Two okay to change?

Great question, and the answer is yes. rustfmt is governed by constraints that don't allow us to change the default formatting produced outside a few exception paths, like genuine bug fixes and iterative formatting support for new syntax. So we can't make any changes to the codebase that would cause the formatting emitted with the default Version::One variant, but the non-default Version::Two isn't subject to those constraints.

@calebcartwright
Copy link
Member

#4711 is also likely somewhat relevant

@ytmimi ytmimi added pr-merge-conflict This PR has merge conflicts that must be resolved p-low labels Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-low pr-merge-conflict This PR has merge conflicts that must be resolved pr-waiting-on-author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants