-
Notifications
You must be signed in to change notification settings - Fork 898
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
Poor formatting of newline return types in existing tests #4373
Comments
I missed this entirely in #4368, but this is the current behavior when I think we need to confirm that the v2 behavior is unintentional/unexpected first, but I personally agree that the suggested indentation would seem to be the expected/preferable option. |
Yes, the cause of the regression (I believe) as mentioned there, is that logic handling correct indentation of the return type was gated behind Version One, which was removed after some time. All of these tests that I could find appear to have been added after the Version One gate was removed. Is there a better way to confirm that the behavior is not intentional? |
Because the gate was added there, we know it was because the author was modifying rustfmt in such a way that was known to break existing formatting and thus required users to explicitly opt into it. The current behavior simply became the default when the gates were removed. Of course that doesn't necessarily mean this part was intentional, as this could have been an unforeseen side effect of some other objective. Best way to try to get back to root intent would be to find the original issue that resulted in the code changes with the gate. Not sure whether that will be easier in this case with git's history or GitHub's search features. Just a side note here, this is a great example of challenges posed by the version gates, especially when they can be very long lived. |
Yeah so #3731 originally added the gate, and #3891 removed it (in src/items.rs). Neither commit includes tests for the particular case that a return type is placed entirely on its own line (i.e. |
I agree, thanks for digging into it! Will assign this to you if you'd like to carry on addressing these, though will want to get confirmation from @topecongiro before merging that this was indeed unintentional as we suspect. |
Sorry for the late reply. Regarding #4366; we should break parameters into multiple lines before anything else if the function signature does not fit in a single line: // rustfmt-max_width: 80
trait GetMetadata {
fn metadata(
loc: ApiEndpointParameterLocation,
) -> Vec<ApiEndpointParameter>;
} If the function has no parameter then we put the return type on its own line: trait GetMetadata {
fn metadata()
-> Vec<ApiEndpointParameter>;
} It is intentional that rustfmt doesn't indent the line starting with |
Hello, is there any update or help wanted on this issue ? Thanks :) |
hi @lroux-at-jellysmack could you expand a bit on your question? Is this something you are encountering using a released version of rustfmt with the |
@calebcartwright is my understanding correct that we should be indenting the return types and we're currently not doing that? |
@calebcartwright: Hello, sorry it was a long time ago and my comment had poor value, I don't remember what problem I encountered at the time :/ |
A number of our existing tests (targeting rustfmt on master) have poor formatting of return types that don't align with the changes made in #4368:
rustfmt/tests/target/issue-3278.rs
Lines 1 to 2 in f817383
rustfmt/tests/target/long-fn-1.rs
Lines 18 to 19 in f817383
rustfmt/tests/target/issue-2672.rs
Lines 30 to 31 in f817383
rustfmt/tests/target/issue-2672.rs
Lines 48 to 49 in f817383
I think these should be fixed to use return type indentation, a formatting regression fixed in #4368. These tests appear to have been added after that regression.
@topecongiro @calebcartwright if this sounds reasonable, feel free to assign me to this issue, I have some ideas of how to fix this and simplify the monolith function that handles function signature formatting (at least w.r.t. return types).
The text was updated successfully, but these errors were encountered: