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

Fix pattern struct fields #5120

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

Conversation

camsteffen
Copy link
Contributor

Fixes #5117

@camsteffen
Copy link
Contributor Author

@calebcartwright can I bother you for your initial thoughts on this? My assumption is that this change is "obviously right" and it would be good to land it before very long.

@calebcartwright
Copy link
Member

My assumption is that this change is "obviously right"

Unfortunately with something as subjective as code style there"s rarely anything that truly, objectively falls into the category of "obviously right". Open to considering this, but to be fully transparent it"s not something I"m likely to get to any time soon.

@camsteffen
Copy link
Contributor Author

I think I can make at least one objective claim, that the current behavior goes against this statement in the style guide:

Patterns should be formatted like their corresponding expressions.

@calebcartwright
Copy link
Member

I think I can make at least one objective claim, that the current behavior goes against this statement in the style guide:

Patterns should be formatted like their corresponding expressions.

Yeah I was a little worried about that, and it"s actually why this may end up being markedly more involved than just the code diff. AFAIK there"s not been a case yet where rustfmt defaults contradict the style guide which would put us in the novel, and unfortunate, position of having to either break compatibility with the style guide or break the formatting stability guarantee.

I realize the proposed changes here are version gated, but it"s something that I"d rather tackle holistically and not have to deal with potentially flip-flopping formatting on those utilizing v2

@camsteffen
Copy link
Contributor Author

That is a pickle. I"d be sad to see the current behavior accepted permanently. This is the one case where I am tempted to reach for rustfmt::skip now and then.

@ytmimi ytmimi added the p-low label Jul 27, 2022
@SUPERCILEX
Copy link

@ytmimi Can this be accepted for version Two?

@ytmimi
Copy link
Contributor

ytmimi commented Dec 5, 2022

@SUPERCILEX apologies for the delay. I think we"re going to hold off on this one for right now. As mentioned in #5120 (comment) we still need to determine which approach we"ll take to resolve this.

@SUPERCILEX
Copy link

Just to make sure I"m understanding the issue correctly, is the following question what"s being debated? When rustfmt violates the style guide, should the resolution break formatting stability or trigger a style guide rewrite to match the current formatting?

I see that question and this version gated PR as somewhat orthogonal. As far as I understand, the point of having multiple formatting versions is to allow the evolution of the style guide. Thus, whatever the answer to the previous question is, it is only relevant to the version one style guide and therefore would not cause flip-flopping for version 2 users (for whom formatting stability would be broken since version 2 isn"t stable yet).

@camsteffen
Copy link
Contributor Author

@calebcartwright Does anything change here since the style guide editions RFC was merged? Could this possibly be a 2024 edition change?

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.

Multi-line struct field values should begin on the same line
4 participants