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

Warn when ignore-tidy-linelength is present, but no lines are too long #60169

Merged
merged 7 commits into from
Apr 23, 2019

Conversation

varkor
Copy link
Member

@varkor varkor commented Apr 22, 2019

It's easy for a // ignore-tidy-linelength to be added when there is a genuine need to ignore a file's line length, but then after refactoring the need is gone, but the tidy directive is not removed. This means that in the future, further editing may accidentally add unnecessarily long lines. This change forces // ignore-tidy-linelength to be used exactly when necessary, to make sure such changes are intentional.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 22, 2019
@varkor
Copy link
Member Author

varkor commented Apr 22, 2019

r? @kennytm

@varkor varkor force-pushed the tidy-unnecessary-ignore-newline branch from 6a6400a to 1da5d9d Compare April 22, 2019 15:42
@kennytm
Copy link
Member

kennytm commented Apr 22, 2019

r=me after Travis passes.

@petrochenkov
Copy link
Contributor

This kind of logic is applicable to all tidy rules.

// ignore-tidy-x && not x -> tidy error

@bors
Copy link
Contributor

bors commented Apr 22, 2019

☔ The latest upstream changes (presumably #60168) made this pull request unmergeable. Please resolve the merge conflicts.

@varkor varkor force-pushed the tidy-unnecessary-ignore-newline branch from 25fb915 to c2895a5 Compare April 22, 2019 19:58
@varkor
Copy link
Member Author

varkor commented Apr 22, 2019

@kennytm: I've updated the PR to also warn when other tidy ignore directives are present, but have no effect. Could you check to make sure the changes are still okay?

@kennytm
Copy link
Member

kennytm commented Apr 23, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Apr 23, 2019

📌 Commit c2895a5 has been approved by kennytm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2019
kennytm added a commit to kennytm/rust that referenced this pull request Apr 23, 2019
…ine, r=kennytm

Warn when ignore-tidy-linelength is present, but no lines are too long

It's easy for a `// ignore-tidy-linelength` to be added when there is a genuine need to ignore a file's line length, but then after refactoring the need is gone, but the tidy directive is not removed. This means that in the future, further editing may accidentally add unnecessarily long lines. This change forces `// ignore-tidy-linelength` to be used exactly when necessary, to make sure such changes are intentional.
@kennytm
Copy link
Member

kennytm commented Apr 23, 2019

@bors r-

This PR failed in the rollup #60189, because #60172 has accidentally removed the CRLF line endings from src/test/run-pass/issues/issue-16278.rs (see #16278). They need to be added back. (Congratulations in detecting a regression? 🙃)

Screenshot 2019-04-23 at 17 50 26-fs8

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 23, 2019
@varkor
Copy link
Member Author

varkor commented Apr 23, 2019

This PR failed in the rollup #60189, because #60172 has accidentally removed the CRLF line endings

Ah, this was very easy to miss — the line endings were converted silently as soon as I modified the file 😬
I've reverted the change and am checking everything now passes again.

@varkor varkor force-pushed the tidy-unnecessary-ignore-newline branch from c2895a5 to 5ab5806 Compare April 23, 2019 11:19
@varkor
Copy link
Member Author

varkor commented Apr 23, 2019

@bors r=kennytm

@bors
Copy link
Contributor

bors commented Apr 23, 2019

📌 Commit 5ab5806 has been approved by kennytm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 23, 2019
Centril added a commit to Centril/rust that referenced this pull request Apr 23, 2019
…ine, r=kennytm

Warn when ignore-tidy-linelength is present, but no lines are too long

It's easy for a `// ignore-tidy-linelength` to be added when there is a genuine need to ignore a file's line length, but then after refactoring the need is gone, but the tidy directive is not removed. This means that in the future, further editing may accidentally add unnecessarily long lines. This change forces `// ignore-tidy-linelength` to be used exactly when necessary, to make sure such changes are intentional.
Centril added a commit to Centril/rust that referenced this pull request Apr 23, 2019
…ine, r=kennytm

Warn when ignore-tidy-linelength is present, but no lines are too long

It's easy for a `// ignore-tidy-linelength` to be added when there is a genuine need to ignore a file's line length, but then after refactoring the need is gone, but the tidy directive is not removed. This means that in the future, further editing may accidentally add unnecessarily long lines. This change forces `// ignore-tidy-linelength` to be used exactly when necessary, to make sure such changes are intentional.
Centril added a commit to Centril/rust that referenced this pull request Apr 23, 2019
…ine, r=kennytm

Warn when ignore-tidy-linelength is present, but no lines are too long

It's easy for a `// ignore-tidy-linelength` to be added when there is a genuine need to ignore a file's line length, but then after refactoring the need is gone, but the tidy directive is not removed. This means that in the future, further editing may accidentally add unnecessarily long lines. This change forces `// ignore-tidy-linelength` to be used exactly when necessary, to make sure such changes are intentional.
Centril added a commit to Centril/rust that referenced this pull request Apr 23, 2019
…ine, r=kennytm

Warn when ignore-tidy-linelength is present, but no lines are too long

It's easy for a `// ignore-tidy-linelength` to be added when there is a genuine need to ignore a file's line length, but then after refactoring the need is gone, but the tidy directive is not removed. This means that in the future, further editing may accidentally add unnecessarily long lines. This change forces `// ignore-tidy-linelength` to be used exactly when necessary, to make sure such changes are intentional.
bors added a commit that referenced this pull request Apr 23, 2019
Rollup of 6 pull requests

Successful merges:

 - #59823 ([wg-async-await] Drop `async fn` arguments in async block )
 - #59839 (Warn on unused results for operation methods on nums)
 - #60146 (Update fonts used by rustdoc)
 - #60169 (Warn when ignore-tidy-linelength is present, but no lines are too long)
 - #60177 (Promote rust comments to rustdoc)
 - #60191 (Add f16c target_feature)

Failed merges:

r? @ghost
@bors bors merged commit 5ab5806 into rust-lang:master Apr 23, 2019
@varkor varkor deleted the tidy-unnecessary-ignore-newline branch April 23, 2019 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants