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

Don't bail out of formatting code blocks if lines are >100 chars #4339

Merged
merged 1 commit into from
Jul 22, 2020

Conversation

ayazhafiz
Copy link
Contributor

Today rustfmt bails when trying to handle a code block that, after
formatting, has lines >100 chars. The reasoning was that if there were
lines longer than 100 chars it wasn't clear if we failed to format or
that was the intended result, so to be cautious the handler would bail.

However, if formatting the snippet fails the code block formatter will
already have jumped out by the time this line-width check is done:

let mut formatted = format_snippet(&snippet, &config_with_unix_newline)?;

So I am not sure that this check is needed, and removing it broke no
tests except one designed explicitly for it. NB w.r.t. that unit test,
formatting a code like

fn main() {
    let expected = "this_line_is_100_characters_long_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx(x, y, z);";
}

would yield no change, so I believe it is also the expected result.

Closes #4325

Today rustfmt bails when trying to handle a code block that, after
formatting, has lines >100 chars. The reasoning was that if there were
lines longer than 100 chars it wasn't clear if we failed to format or
that was the intended result, so to be cautious the handler would bail.

However, if formatting the snippet fails the code block formatter will
already have jumped out by the time this line-width check is done:

https://github.com/rust-lang/rustfmt/blob/ff57cc6293ce450380dbb6da6a1a3656dbae538d/src/formatting/util.rs#L81

So I am not sure that this check is needed, and removing it broke no
tests except one designed explicitly for it. NB w.r.t. that unit test,
formatting a code like

```
fn main() {
    let expected = "this_line_is_100_characters_long_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx(x, y, z);";
}
```

would yield no change, so I believe it is also the expected result.

Closes rust-lang#4325
Copy link
Contributor

@topecongiro topecongiro left a comment

Choose a reason for hiding this comment

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

I am a bit worried about the side effect this PR may introduce, but still, I think the benefit of fixing the #4325 outweighs the potential side effect risk. So LGTM.

@topecongiro topecongiro merged commit ebf0de7 into rust-lang:master Jul 22, 2020
@topecongiro
Copy link
Contributor

Thank you 🎉

@ayazhafiz
Copy link
Contributor Author

I am a bit worried about the side effect this PR may introduce

Yes, I agree. If/when we get an issue because of this change, then we can concretely point and say why this cannot be changed.

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.

Strange formatting with nested macro_rules! and long comment
3 participants