Adding lint test for excessive LOC.

[avborhanian]

This is a WIP for #2377. Just wanted to pull in because I had a few questions:

1. Is it okay that I'm approaching this via counting by looking at each line in the snippet instead of looking at the AST tree? If there's another way to do it, I want to make sure I'm doing the correct way, but I wasn't sure since the output AST JSON doesn't seem to contain whitespace.

2. My function is definitely going to trigger the lint, so also wanted to see if there was something obvious I could do to reduce it.

3. Are the two tests fine, or is there something obvious I'm missing?

4. Obviously bigger question - am I approaching the line count correctly. Current strategy is count a line if it contains some code, so skip if it's just comments or empty.

[oli-obk]

1. yes
2. I'll comment inline
3. please add them to their own file. We're trying to have many small tests files instead of few large ones
4. makes sense

[avborhanian]

Are there any more changes I should consider making?

[oli-obk]

Let's retrigger CI

[mati865]

Triggered itself in matches.rs test, you will have to allow it there:

+error: This function has a large number of lines.
+   --> $DIR/matches.rs:126:1
+    |
+126 | / fn match_wild_err_arm() {
+127 | |     let x: Result<i32, &str> = Ok(3);
+128 | |
+129 | |     match x {
+...   |
+207 | |     }
+208 | | }
+    | |_^
+    |
+    = note: `-D too-many-lines` implied by `-D warnings`

---

Your test doesn't work, maybe you have to add two prints so function body exceeds 51?

diff of stderr:
-error: This function has a large number of lines.
-   --> $DIR/functions_maxlines.rs:57:1
-    |
-57  | / fn bad_lines() {
-58  | |     println!("This is bad.");
-59  | |     println!("This is bad.");
-60  | |     println!("This is bad.");
-...   |
-107 | |     println!("This is bad.");
-108 | | }
-    | |_^
-    |
-    = note: `-D too-many-lines` implied by `-D warnings`
-
-error: aborting due to previous error

[oli-obk]

Dogfood tests die on this :D Clippy itself violates that rule so many times... https://travis-ci.org/rust-lang-nursery/rust-clippy/jobs/397138977#L1006

Not sure if we should increase the limit, allow the lint inside clippy_lints, make it a restriction lint, or any combination of the above.

[mati865]

@oli-obk I just noticed restriction group is not mentioned in README.md, guess it should be there.

[avborhanian]

So for those tests, the line counts were on average 101 lines. Median was 68 lines. Mode was 71 lines. If I increased the limit to trigger on line counts greater than or equal to 101 lines, it dropped the number of failures from 72 to 17, in 14 different files instead of 45 different files.

I think it would make sense to make it allowed inside clippy_lints. I'm not sure what the restriction limit does though, not as familiar with that.

[oli-obk]

The restriction group means it's a lint that you have to enable explicitly.

Even with the guidelines from the clippy RFC I'm not sure in which group to place it though ;) It has no false positives, but it can be inactionable (imagine a function exhaustively matching on a 50 variant enum with a few lines of actions per variant).

The 17 functions that you found, did they look like they should be split up?

[fabric-and-ink]

Is it possible to deactivate a lint for a specific block? Then we could silence the lint for those 17 functions and postpone a refactoring.

[oli-obk]

Sure, just allow it. Or just make it a restriction lint so it needs explicit warn attributes to get turned on.

[phansch]

@avborhanian would you like to finish up this PR? It would need a rebase and the 17 functions would have to be annotated with #[allow(clippy::too_many_lines)] to get started.

[flip1995]

Ping from triage @avborhanian: It looks like this PR hasn't received any updates in a while, so I'm closing it per our new guidelines. Thank you for your contributions and please feel free to re-open in the future.

[avborhanian]

Hey! Sorry I didn't respond earlier @phansch. I did the rebase, and it looks like it expanded to include 3 more functions, so I added the lint to those as well. Also had to move the lint from the clippy::all list to the clippy::pedantic list. What do I need to do to reopen the PR?

[flip1995]

Thanks for the update @avborhanian! It seems that the rebase somehow failed. Could you try to fix this? After that we're happy to make another review.

[avborhanian]

Just curious, is there a way to adjust a configuration variable at the individual test level? Feel like it could be useful for either decreasing the lines I'm testing/making sure different values work as expected.

[flip1995]

Yes there is! You can look at tests/ui-toml for examples.

[phansch]

Thanks for your patience! Everything LGTM now.

@bors r+

[bors]

📋 Looks like this PR is still in progress, ignoring approval.

Hint: Remove WIP from this PR's title when it is ready for review.

[avborhanian]

I was about to say the same thing - 7 month long pull request, thanks for the patience. :D

[phansch]

@bors r+ 🎊

[bors]

📌 Commit ac9472d has been approved by phansch

[bors]

⌛ Testing commit ac9472d with merge 27b5dd8...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: phansch
Pushing 27b5dd8 to master...