Skip to content

Line exceeded maximum length warning caused by CRLF #1335

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

Closed
iliekturtles opened this issue Feb 25, 2017 · 5 comments
Closed

Line exceeded maximum length warning caused by CRLF #1335

iliekturtles opened this issue Feb 25, 2017 · 5 comments
Labels
bug Panic, non-idempotency, invalid code, etc. good first issue Issues up for grabs, also good candidates for new rustfmt contributors

Comments

@iliekturtles
Copy link
Contributor

The code block below gives the following warning when the file is saved with Window's native CRLF line ending: Rustfmt failed at S:\uom\src\system.rs:143: line exceeded maximum length (maximum: 100, found: 101) (sorry). Saving the file with a LF line ending silences the warning. rustfmt must be counting the CR (\r) as part of the line length.

                impl<$($name,)+ $($symbol,)+ V> $crate::stdlib::ops::$Trait<BaseUnits<$($name,)+ V>>
@nrc nrc added bug Panic, non-idempotency, invalid code, etc. good first issue Issues up for grabs, also good candidates for new rustfmt contributors labels Feb 26, 2017
@ghost
Copy link

ghost commented Feb 28, 2017

@iliekturtles It indeed does : https://github.com/rust-lang-nursery/rustfmt/blob/master/src/lib.rs#L473

However, since there's a special case for \r, it seems it's done on purpose. I don't know why, not familiar with this package, let's hope someone more experienced with this package can help us :)

@iliekturtles
Copy link
Contributor Author

After digging through the blame I see that commit 598fcdd, which references #717, added the line_len increment. This brings up a bigger question about why len_utf8 is being used which reports character width in bytes. What should really be counted graphemes? How does that handle combining characters?

@ghost
Copy link

ghost commented Feb 28, 2017

@iliekturtles Good catch ! It actually does handle them quite poorly.

This code (length of the line is 70 grapheme-long):

fn main() {
    println!("ééééééééééééééééééééééééééééééééééééééééééééééééééééé");
}

produces :

Rustfmt failed at combined_characters.rs:2: line exceeded maximum length (maximum: 100, found: 123) (sorry)

Let's open an issue for that and link it to this one ?

@nrc
Copy link
Member

nrc commented Feb 28, 2017

The special case code for \n should also count \r. That should be an easy fix.

Rustfmt just doesn't handle unicode > 8 bits well at all (currently the oldest issue in this repo - #6).

@ghost
Copy link

ghost commented Feb 28, 2017

Thanks for the info @nrc . I'll crate a PR accordingly to your suggestion

iliekturtles added a commit to iliekturtles/rustfmt that referenced this issue Apr 2, 2017
Resolves rust-lang#1335. Does not attempt to handle a \r not followed by a \n nor
attempt to handle Unicode intracacies including zero-width or multi-byte
characters.
iliekturtles added a commit to iliekturtles/rustfmt that referenced this issue Apr 2, 2017
Resolves rust-lang#1335. Does not attempt to handle a `\r` not followed by a `\n` nor
attempt to handle Unicode intricacies including zero-width or multi-byte
characters.
iliekturtles added a commit to iliekturtles/rustfmt that referenced this issue Apr 2, 2017
Resolves rust-lang#1335. Does not attempt to handle a `\r` not followed by a `\n` nor
attempt to handle Unicode intricacies (rust-lang#6) including zero-width or multi-byte
characters.
@nrc nrc closed this as completed in #1435 Apr 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Panic, non-idempotency, invalid code, etc. good first issue Issues up for grabs, also good candidates for new rustfmt contributors
Projects
None yet
Development

No branches or pull requests

2 participants