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

Do not normalize vertical spaces unless they are between items #4295

Conversation

topecongiro
Copy link
Contributor

@topecongiro topecongiro commented Jun 30, 2020

With this PR, we no longer normalize the number of vertical lines unless those are in between items.

Since this PR changes the default formatting style, I would like to include this PR to the 2.0 release.

Close #2954.

- We no longer normalize number of vertical spaces between statements.
- Add a test
@@ -7,24 +7,28 @@ fn main() {
let bar = ();
}


Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am adding blank lines to this and subsequent targets to match the number of blank lines in the corresponding source.

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Changes LGTM. One minor thought about renaming things to use newline consistently instead of the mix with vertical_spaces, but not a big deal

@@ -63,13 +64,15 @@ impl<'a> FmtVisitor<'a> {
}
let indent = this.block_indent.to_string(config);
this.push_str(&indent);
})
});
self.normalize_vertical_spaces = false;
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the borderline pun, but could we normalize the variable/function names and replace the old vertical_spaces with newline consistently everywhere?

Suggested change
self.normalize_vertical_spaces = false;
self.normalize_newline_counts = false;

newline_count
}

fn push_vertical_spaces(&mut self, mut newline_count: usize) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn push_vertical_spaces(&mut self, mut newline_count: usize) {
fn push_newlines(&mut self, mut newline_count: usize) {

@topecongiro topecongiro merged commit 7159095 into rust-lang:master Jul 3, 2020
@topecongiro topecongiro deleted the normalize-vertical-spaces-between-items branch July 3, 2020 02:13
@dcow
Copy link

dcow commented May 19, 2022

I'm running the latest nightly rust and formatting files using cargo +nightly fmt and I still experience this bug that was merged 2 years ago. What am I doing wrong?

@ytmimi
Copy link
Contributor

ytmimi commented May 19, 2022

@dcow You're not doing anything wrong. This PR was merged into the rustfmt-2.0.0-rc.2 branch (I ran git branch -a --contains=7159095 to verify that).

There are a bunch of other backport-triage PRs that need to be looked at. Some are simple cherry-picks, but others are more involved due to the two branches diverging. Others are complicated by the fact that they were introduced at a time where a major version of rustfmt was in the works (now on pause) so they could more liberally introduce breaking changes for the new release, which we need to be mindful of when trying to backport them into 1.x branch.

That being said, if you're still having issues I'd suggest opening up a new issue with a short code snippet that we can use to reproduce your issue, and one that we can test against 1.x and the rustfmt-2.0.0-rc.2 branch

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.

Is blank_lines_lower_bound correct?
5 participants