-
Notifications
You must be signed in to change notification settings - Fork 10.8k
feat: Add clippy lints #269
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea! I think that we should have a bit more information about how to run these kinds of exercises, probably in the comments of clippy1.rs
. Something explaining what exactly Clippy is would help too.
fs::write(CLIPPY_CARGO_TOML_PATH, cargo_toml) | ||
.expect("Failed to write 📎 Clippy 📎 Cargo.toml file."); | ||
Command::new("cargo") | ||
.args(&["clippy", "--manifest-path", CLIPPY_CARGO_TOML_PATH]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would imply that you have to have Clippy installed, right? As far as I know, Clippy isn't added as a default Rustup component, so you'd have to mention somewhere that you need to explicitly do rustup component add clippy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right.
I added the Clippy installation to the install scripts. I assume this should be good then?
@@ -2,3 +2,5 @@ | |||
target/ | |||
**/*.rs.bk | |||
.DS_Store | |||
exercises/clippy/Cargo.toml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming the separate Cargo.toml is so that Clippy doesn't lint the entire project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was hoping that there is an option to invoke Clippy without Cargo and without a Cargo.toml and just pass it the bin with an option, but it seems that this is not possible.
This is why I'm generating a custom Cargo.toml just to invoke Clippy with it
acc4e28
to
25de8fd
Compare
I added a section to the first Clippy example. I'm not sure if this is enough or if a README should be added. |
I would say just to be sure, add a README file that explains that you need Clippy installed for these exercises (but that, if you'd used the installation script, it would have already installed it for you). Also, a link to more information about Clippy would be good to have in that file. Other than that, looks good to me! |
☔ The latest upstream changes (presumably #277) made this pull request unmergeable. Please resolve the merge conflicts. |
- adds a new 'clippy' category for exercises - clippy exercises should throw no warnings - install script now also installs clippy is related to rust-lang/rust-clippy#2604
I fixed the conflicts and added a README |
@bors: r+ |
📌 Commit 1e2fd9c has been approved by |
☀️ Test successful - checks-travis |
@Tarnadas |
feat: Add clippy lints This is a feature PR which adds the possiblity to create clippy exercises. Clippy has many awesome linting rules, which can give a deeper understanding about the Rust programming language, therefor I made this PR.
feat: Add clippy lints This is a feature PR which adds the possiblity to create clippy exercises. Clippy has many awesome linting rules, which can give a deeper understanding about the Rust programming language, therefor I made this PR.
This is a feature PR which adds the possiblity to create clippy exercises.
Clippy has many awesome linting rules, which can give a deeper understanding about the Rust programming language, therefor I made this PR.