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.

Sign up for GitHub

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

Jump to bottom

Fix all Clippy suggestions (but not add it to CI 🙃) #7574

Merged
merged 16 commits into from
Nov 15, 2019
Merged

Fix all Clippy suggestions (but not add it to CI 🙃) #7574

merged 16 commits into from
Nov 15, 2019

Conversation

igor-makarov
Copy link
Contributor

@igor-makarov igor-makarov commented Nov 9, 2019 •
edited
Loading

This PR adds a clippy job to the Azure Pipelines CI.

In addition to adding the job, I made sure to fix all the lints in all the packages.

I'm new to Rust, so please check my code modifications extra carefully 😂

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 9, 2019
@igor-makarov igor-makarov changed the title remove too_many_arguments Add Clippy checks to CI Nov 9, 2019
@igor-makarov igor-makarov force-pushed the clippy-fixes branch 7 times, most recently from ccd0e48 to 9dde707 Compare November 10, 2019 14:32
@igor-makarov igor-makarov marked this pull request as ready for review November 10, 2019 15:33
@igor-makarov
Copy link
Contributor Author

Hi @Eh2406!

I'm ready for review.

I'm new to Rust, so please check my code modifications extra carefully 😂

@Eh2406
Copy link
Contributor

Eh2406 commented Nov 10, 2019 via email

@igor-makarov
Copy link
Contributor Author

You got me a bit nervous 😅

Opinionated how? Is he against it?

@alexcrichton
Copy link
Member

Thanks for the PR!

I would personally prefer to not run clippy on CI. My personal opinion is that it generates far too much noise to be useful and encourages the wrong antipattern by sprinkling #[allow] annotations with seemingly random names all throughout the codebase. I don't mind taking PRs to merge improvements every now and then, but I do not want to force all authors for all PRs to work with the pedantry of clippy.

This is just my personal opinion, though, and others on the team may feel different.

@igor-makarov
Copy link
Contributor Author

Hi @alexcrichton!

My previous experience in open source was contributing to CocoaPods and implementing its CDN registry client/static site. During that work, I've become very used to Rubocop, the Ruby linter. They have it on the CI and it mainly helps with having the code style be consistent.

Allow me to address the concerns you raised:

  • The anti-pattern of allowing violations can and should be enforced by reviewers. Generally speaking, a conservative policy of "no #allowss unless absolutely necessary" goes a long way.
  • The pedantry of Clippy can be reduced by adding global #allows`s. In fact, I believe lots of them are already there!
  • There's currently less pedantry than it seems. After turning Clippy on CI, I've managed to get rid of all the warnings in a matter of a couple of hours. Maintaining this state should be even less arduous.

To summarize - it will help enforce a consistent style and cleaner code. If rustfmt is enforced, Clippy might as well. Plus, it's way easier to appease than the borrow checker 😂 As Rust continues to gain popularity, the number of contributors will increase. We might as well be ready.

Also, having a completely problem-free output in VS Code is exhilarating!

@alexcrichton
Copy link
Member

I understand the value of consistency, yeah, and I can see how clippy tries to nudge to more consistent code. That being said I disagree with many of clippy's more pedantic lints, I personally do not like having a massive list of #[allow] at the top of the crate for just clippy, and already in this PR lots of #[allow] is creeping in.

Again, this is just my personal opinion, but I trust development and ongoing maintenance of rustfmt more than I trust the development and maintenance of clippy. The rustfmt style was agreed upon the community through an RFC process and agreement amongst a team, whereas I believe the clippy lints have not. They're simply "if someone implemented it we threw it in and r+'d it".

I should point out though that it's pretty unlikely that I'm going to be convinced to like clippy any time soon. I am not the only maintainer of this project though and while I would push back against adding clippy to CI if everyone else would like it then I'd of course be ok. I'd like more team input about running this on CI first.

ehuss
ehuss reviewed Nov 11, 2019
View reviewed changes
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

I'm not too enthusiastic about adding clippy to CI. It can be too fussy, with too many false positives. I think occasional cleanup PRs that address some of the warnings are good, though, as long as they are not too large and not bending over too much to accommodate clippy's choices.

It might also be a good idea to file issues on clippy for all the false positives.

crates/cargo-test-support/src/registry.rs Outdated Show resolved Hide resolved
crates/cargo-test-support/src/registry.rs Outdated Show resolved Hide resolved
crates/cargo-test-support/src/registry.rs Outdated Show resolved Hide resolved
@igor-makarov
Copy link
Contributor Author

So what do you suggest the next steps should be for this PR?

Also, are you folks okay with rebasing and editing commit history and/or deleting commits from a PR? Or should I add commits on top even if they're basically deletions?

@bors
Copy link
Collaborator

bors commented Nov 11, 2019

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

@alexcrichton
Copy link
Member

Yes rebasing and editing the history is fine. I don't think we want to land the CI support, but landing changes which take into account clippy lints is fine. It's preferred to put any #[allow], if required, at the top of the crate rather than on the use site.

@igor-makarov
Copy link
Contributor Author

I've revised the suggested changes. For now, I've kept the Clippy CI because it's the easiest way for me to know that all my changes are good, Clippy-wise 🙃

What other things need to happen for allowing the linting to be performed by CI? The benefits are clear and I think shouldn't be ignored.

@Eh2406
Copy link
Contributor

Eh2406 commented Nov 14, 2019

We discussed this at the Cargo team meeting yesterday. We are not going to run Clippy in azure at this time. If you can add a commit removing the changes to azure-pipelines.yml we are happy to merge the other changes. (I'd prefer to keep it in the history as it looks like a valuable list of all the things that needs to be done to check with clippy.) We don't think it is controversial to say that clippy is at times pedantic and needs a large numbers of #![allow(clippy:: to make acceptable. We discussed some changes that may improve the signal/noise, but it is more an issue for the clippy devs. We acknowledge that it is not a good look that official Rust projects are not willing to dog food the official linter. So we will be bringing this up with the broader Dev-Tools team. It may be something that is best hashed out in person, so we may rezerve time at the Rust All Hands for this discussion.

@igor-makarov igor-makarov changed the title Add Clippy checks to CI Fix all Clippy suggestions (but not add it to CI 🙃) Nov 15, 2019
@igor-makarov
Copy link
Contributor Author

@Eh2406 Done!

@Eh2406
Copy link
Contributor

Eh2406 commented Nov 15, 2019

Thank you for working on this!

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 15, 2019

📌 Commit 1432651 has been approved by Eh2406

@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 Nov 15, 2019
@bors
Copy link
Collaborator

bors commented Nov 15, 2019

⌛ Testing commit 1432651 with merge 3738e1d...

bors added a commit that referenced this pull request Nov 15, 2019
@bors
Auto merge of #7574 - igor-makarov:clippy-fixes, r=Eh2406
3738e1d 
Fix all Clippy suggestions (but not add it to CI 🙃)

This PR adds a `clippy` job to the Azure Pipelines CI.

In addition to adding the job, I made sure to fix all the lints in all the packages.

I'm new to Rust, so please check my code modifications extra carefully 😂
@bors
Copy link
Collaborator

bors commented Nov 15, 2019

☀️ Test successful - checks-azure
Approved by: Eh2406
Pushing 3738e1d to master...

@bors bors merged commit 1432651 into rust-lang:master Nov 15, 2019
@ehuss ehuss mentioned this pull request Nov 25, 2019
Merged
bors added a commit to rust-lang/rust that referenced this pull request Nov 25, 2019
@bors
Auto merge of #66748 - ehuss:update-cargo-books, r=alexcrichton
91642e3 
Update cargo, rls, books.

## nomicon

1 commits in 58e36e0e08dec5a379ac568827c058e25990d6cd..041c46e692a2592853aeca132c8dfe8eb5a79a9e
2019-10-30 08:14:24 -0500 to 2019-11-20 16:46:45 +0100
- Update unsafe-code-guidelines link (rust-lang/nomicon#175)

## cargo

15 commits in 8280633db680dec5bfe1de25156d1a1d53e6d190..750cb1482e4d0e74822cded7ab8b3c677ed8b041
2019-11-11 23:17:05 +0000 to 2019-11-23 23:06:36 +0000
- Some random comments and docstrings. (rust-lang/cargo#7625)
- Add value OUT_DIR to build-script-executed JSON message (rust-lang/cargo#7622)
- Update documentation for custom target dependencies. (rust-lang/cargo#7623)
- Document private items for binary crates by default (rust-lang/cargo#7593)
- Extend documentation on security concerns of crate names in a registry. (rust-lang/cargo#7616)
- Stabilize install-upgrade. (rust-lang/cargo#7560)
- Turn the new lock file format on by default (rust-lang/cargo#7579)
- bump im-rc version (rust-lang/cargo#7609)
- Ignore file lock errors if unsupported, on Windows (rust-lang/cargo#7602)
- Add hack for fwdansi change. (rust-lang/cargo#7607)
- Document Cargo's JSON output. (rust-lang/cargo#7595)
- Remove "cargo login" from user input when asking for login token. (rust-lang/cargo#7588)
- Fix all Clippy suggestions (but not add it to CI 🙃) (rust-lang/cargo#7574)
- Add kind/platform info to `cargo metadata` (rust-lang/cargo#7132)
- Update core-foundation requirement from 0.6.0 to 0.7.0 (rust-lang/cargo#7585)

## reference

2 commits in 45558c4..9e843ae
2019-11-08 14:47:35 +0100 to 2019-11-24 17:44:04 +0100
- Minor never type additions. (rust-lang/reference#723)
- Update associated-items.md.  "it"->is (rust-lang/reference#721)

## book

3 commits in e79dd62aa63396714278d484d91d48826737f47f..81ebaa2a3f88d4d106516c489682e64cacba4f60
2019-10-30 07:33:12 -0500 to 2019-11-15 08:30:04 -0800
- small fix ch04-03 & code block typo ch07-02 (rust-lang/book#2138)
- Adapt content of Chapter 16.3 in order to be consistent with improved compiler message (rust-lang/book#1779)
- [Rust 1.35] Remove FnBox and use builtin impl FnOnce for Box<FnOnce()> instead. (rust-lang/book#1906)

## rls

3 commits in 5db91c7b94ca81eead6b25bcf6196b869a44ece0..9ec2b8cb57c87517bcb506ac302eae339ffa2025
2019-10-30 16:04:39 +0100 to 2019-11-24 23:16:11 +0100
- Fix test for latest nightly. (rust-lang/rls#1595)
- doc: contributing: Remove outdated LSP extension (rust-lang/rls#1594)
- Update cargo. (rust-lang/rls#1591)

## rust-by-example

1 commits in dcee312c66267eb5a2f6f1561354003950e29105..4835e025826729827a94fdeb7cb85fed288d08bb
2019-10-31 11:26:53 -0300 to 2019-11-14 09:20:43 -0300
- crates: fix suggested value for --crate-type flag (rust-lang/rust-by-example#1292)

## edition-guide

1 commits in f553fb26c60c4623ea88a1cfe731eafe0643ce34..6601cab4666596494a569f94aa63b7b3230e9769
2019-10-30 08:27:42 -0500 to 2019-11-22 12:08:58 -0500
- Remove final nursery reference
@ehuss ehuss added this to the 1.41.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehuss ehuss ehuss left review comments

Assignees

@Eh2406 Eh2406

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.41.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@igor-makarov @rust-highfive @Eh2406 @alexcrichton @bors @ehuss