Skip to content

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Aug 26, 2017

I recently wrote some tests for a clippy lint, copied the (uppercase) lint name into my test file and forgot to toggle the case. This PR adds a suggestion that would have saved me 10 minutes of debugging, so it's likely a net win 🙂 . Also it adds a UI test for the unknown_lints lint.

@rust-highfive
Copy link
Contributor

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb
Copy link
Member

eddyb commented Aug 26, 2017

r? @nikomatsakis Did someone add a check for "suggestion" in @rust-highfive to assign me?!

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Aug 26, 2017
@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 28, 2017
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Feels like a rather niche use case, but it's only a few lines of code, so... probably fine. Still, let's at least clean up the tests a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect to see this be #[allow(FOO_BAR)], to show that the suggestion does not fire for nonsense lints...is there a reason you chose foo_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.

This is testing the default behavior as a baseline.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment explaining the purpose of this test is to test the "lint case" suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@llogiq
Copy link
Contributor Author

llogiq commented Aug 29, 2017

I will re-jiggle the tests and add some comments when I get to my PC.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 29, 2017

📌 Commit fa6c605 has been approved by nikomatsakis

@bors
Copy link
Collaborator

bors commented Aug 29, 2017

⌛ Testing commit fa6c605e00a72790ae6a34dc811a08262de6a548 with merge b26980dfb2223bcd2e23fcdefaee8816a6d3724b...

@bors
Copy link
Collaborator

bors commented Aug 29, 2017

💔 Test failed - status-travis

Copy link
Member

Choose a reason for hiding this comment

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

The line numbers are wrong.

-warning: unknown lint: `foo_bar`
-  --> $DIR/not_found.rs:11:9
+warning: unknown lint: `FOO_BAR`
+  --> $DIR/not_found.rs:14:9
    |
 14 | #[allow(FOO_BAR)]
    |         ^^^^^^^
    |
    = note: #[warn(unknown_lints)] on by default
 
 warning: unknown lint: `DEAD_CODE`
-  --> $DIR/not_found.rs:12:8
+  --> $DIR/not_found.rs:16:8
    |
 16 | #[warn(DEAD_CODE)]
    |        ^^^^^^^^^ help: lowercase the lint name: `dead_code`
 
 warning: unknown lint: `Warnings`
-  --> $DIR/not_found.rs:13:8
+  --> $DIR/not_found.rs:18:8
    |
 18 | #[deny(Warnings)]
    |        ^^^^^^^^ help: lowercase the lint name: `warnings`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sorry about that. Me, at a bus stop, with slow, intermittent 3G connection trying to fake compiletest (because rustbuild doesn't like connection timeouts). Let's try that again.

@arielb1 arielb1 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 29, 2017
@nikomatsakis
Copy link
Contributor

Travis gives this result:

[00:40:37]  warning: unknown lint: `FOO_BAR`

[00:40:37] -  --> $DIR/not_found.rs:11:9

[00:40:37] +  --> $DIR/not_found.rs:14:9

[00:40:37]     |

[00:40:37]  14 | #[allow(FOO_BAR)]

[00:40:37]     |         ^^^^^^^

[00:40:37]     |

[00:40:37]     = note: #[warn(unknown_lints)] on by default

[00:40:37]  

[00:40:37]  warning: unknown lint: `DEAD_CODE`

[00:40:37]    --> $DIR/not_found.rs:16:8

[00:40:37]     |

[00:40:37]  16 | #[warn(DEAD_CODE)]

[00:40:37]     |        ^^^^^^^^^ help: lowercase the lint name: `dead_code`

[00:40:37]  

[00:40:37]  warning: unknown lint: `Warnings`

[00:40:37]    --> $DIR/not_found.rs:18:8

[00:40:37]     |

[00:40:37]  18 | #[deny(Warnings)]

[00:40:37]     |        ^^^^^^^^ help: lowercase the lint name: `warnings`

@llogiq
Copy link
Contributor Author

llogiq commented Aug 31, 2017

What the git? I wanted to rebase this with the first comment, not with the stuff you see there. At least the tests no longer fail.

@llogiq
Copy link
Contributor Author

llogiq commented Aug 31, 2017

@nikomatsakis sorry for wasting your time and thank you for the thorough review. I only fixed the commit message, the tests should run through as is.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 1, 2017

📌 Commit ba643fa has been approved by nikomatsakis

@nikomatsakis
Copy link
Contributor

@llogiq not to worry! travis looks green now =)

@bors
Copy link
Collaborator

bors commented Sep 1, 2017

⌛ Testing commit ba643fa with merge 34e6788ba1edd310e5e66371f592fe9c1b744a03...

@bors
Copy link
Collaborator

bors commented Sep 1, 2017

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

@bors: retry

  • Travis osx weirdness

@bors
Copy link
Collaborator

bors commented Sep 2, 2017

⌛ Testing commit ba643fa with merge efceda2...

bors added a commit that referenced this pull request Sep 2, 2017
add a lowercase suggestion to unknown_lints

I recently wrote some tests for a clippy lint, copied the (uppercase) lint name into my test file and forgot to toggle the case. This PR adds a suggestion that would have saved me 10 minutes of debugging, so it's likely a net win 🙂 . Also it adds a UI test for the `unknown_lints` lint.
@bors
Copy link
Collaborator

bors commented Sep 2, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing efceda2 to master...

@bors bors merged commit ba643fa into rust-lang:master Sep 2, 2017
@llogiq llogiq deleted the lowercase-lints branch September 2, 2017 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants