Enable clippy in Cargo.toml, fix associated issues#8
Enable clippy in Cargo.toml, fix associated issues#8DeflateAwning wants to merge 3 commits intorp-rs:mainfrom
Conversation
Cargo.toml
Outdated
| keywords = ["embedded", "rp235x", "hal", "rust", "template"] | ||
| categories = ["embedded", "hardware-support"] |
There was a problem hiding this comment.
I don't think all of these keywords/categories make sense for a template?
As a user of this template, I would be surprised if my binary crate was tagged with "template" or "hardware support"
There was a problem hiding this comment.
Okay, fair enough, I agree! Will remove those.
| { | ||
| "rust-analyzer.cargo.target": "thumbv8m.main-none-eabihf", | ||
| "rust-analyzer.check.allTargets": false, | ||
| "rust-analyzer.check.command": "clippy", |
There was a problem hiding this comment.
I think this change is controversial. We should discuss whether this is a sensible default for all users in the rp-rs matrix channel.
There was a problem hiding this comment.
This is difficult to answer. I'm not even sure what my personal preference would be, I often switch between clippy and check.
There was a problem hiding this comment.
The current CI config fails if there's linting issues (checked with clippy), so it's important that the default IDE configuration points out these issues before pushing. This change simply reconciles the IDE config with the CI config.
| [lints.clippy] | ||
| all = { level = "warn", priority = -1 } | ||
| pedantic = { level = "warn", priority = -1 } | ||
| multiple_crate_versions = "allow" | ||
|
|
||
| nursery = { level = "warn", priority = -1 } | ||
| cargo = { level = "warn", priority = -1 } | ||
| # restriction = { level = "warn", priority = -1 } |
There was a problem hiding this comment.
Why these particular settings?
This should be discussed in the rp-rs matrix channel too.
There was a problem hiding this comment.
These are basically all the stardard lints in an "all lints enabled" repo. It should be easy for people to start with a template that passes all lints, and they can disable linting if they choose. It's way more of a pain to offer a template that fails linting and that has to be fixed in every generated repo.
Additionally, I suspect that this template is used by a decent number of people who are still new in Rust. Clippy is a fantastic way to offer "here's the best Rust way to do it" training to people as they write code. Again, easier to disable than enable in a generated repo.
priority = -1 is used so that overrides (e.g., multiple_crate_versions) can be used. That override exists here because it fails currently, and doesn't seem like a huge deal (basically complains that two versions of a single crate are built in the dependency graph). Could be fixed in a future PR if we cared; didn't dig too far into it.
There was a problem hiding this comment.
I think those are somewhat orthogonal issues.
Having the template lint free with the most strict/pedantic selection makes sense.
Forcing everyone setting up their project from the template to this level does not.
The template is there as a quick starter.
If one has to spend time fiddling with the linter setting to get going, that's counter productive IMHO.
There was a problem hiding this comment.
I think those are somewhat orthogonal issues.
Having the template lint free with the most strict/pedantic selection makes sense.
Forcing everyone setting up their project from the template to this level does not.
The template is there as a quick starter.
If one has to spend time fiddling with the linter setting to get going, that's counter productive IMHO.
There was a problem hiding this comment.
I'd suggest that writing code that satisfies the linter is the correct way to get going. Writing shitty code is a step of a lot of people's learning paths, but it certainly doesn't have to be.
I wish someone gave me a linter when I was learning to code (in Python and C) - would have progressed me along a fair bit faster.
Why do you think the default action of users of the template would be to "fiddle with the linter" (which I assume you're implying means to turn off the linter)?
| name: CI Checks | ||
|
|
||
| on: [push, pull_request] | ||
| on: [push, pull_request, workflow_dispatch] |
There was a problem hiding this comment.
You never mention this change in the PR notes either
There was a problem hiding this comment.
Being able to re-run CI arbitrarily is helpful in cases of failures, checking against the latest version of Rust (esp. when new linting rules are added), etc. Pretty minor note. I'd consider it a misconfiguration to not have this in basically all repos.
4 participants
No description provided.