Skip to content

Enable clippy in Cargo.toml, fix associated issues #8

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

Open
DeflateAwning wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Enable clippy in Cargo.toml, fix associated issues #8

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0eae8d0
Enable clippy in Cargo.toml, fix associated issues
DeflateAwning Dec 13, 2025
8c8cc69
ci: Enable workflow_dispatch trigger
DeflateAwning Dec 20, 2025
d484bf4
Update keywords and categories in Cargo.toml from review
DeflateAwning Dec 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .github/workflows/ci_checks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name: CI Checks

on: [push, pull_request]
on: [push, pull_request, workflow_dispatch]
Copy link
Member

@9names 9names Dec 20, 2025

Choose a reason for hiding this comment

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

You never mention this change in the PR notes either

Copy link
Contributor Author

@DeflateAwning DeflateAwning Dec 21, 2025

Choose a reason for hiding this comment

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

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.


env:
CARGO_TERM_COLOR: always
Expand All @@ -25,7 +25,7 @@ jobs:
- windows-latest
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v5
- uses: dtolnay/rust-toolchain@master
with:
toolchain: ${{ matrix.rust }}
Expand All @@ -37,7 +37,7 @@ jobs:
name: Linting
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v5
with:
submodules: true
- uses: dtolnay/rust-toolchain@stable
Expand All @@ -49,7 +49,7 @@ jobs:
name: Formatting
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v5
with:
submodules: true
- uses: dtolnay/rust-toolchain@stable
Expand Down
3 changes: 2 additions & 1 deletion .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{
"rust-analyzer.cargo.target": "thumbv8m.main-none-eabihf",
"rust-analyzer.check.allTargets": false,
"rust-analyzer.check.command": "clippy",
Copy link
Member

@9names 9names Dec 20, 2025

Choose a reason for hiding this comment

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

I think this change is controversial. We should discuss whether this is a sensible default for all users in the rp-rs matrix channel.

Copy link
Member

@jannic jannic Dec 20, 2025

Choose a reason for hiding this comment

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

This is difficult to answer. I'm not even sure what my personal preference would be, I often switch between clippy and check.

Copy link
Contributor Author

@DeflateAwning DeflateAwning Dec 21, 2025

Choose a reason for hiding this comment

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

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.

"editor.formatOnSave": true
}
}
13 changes: 13 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ edition = "2024"
name = "rp235x-project-template"
version = "0.1.0"
license = "MIT OR Apache-2.0"
description = "Project template for rp235x-hal"
repository = "https://github.com/rp-rs/rp235x-project-template"
keywords = ["embedded", "rp235x", "hal", "rust", "template"]
categories = ["embedded", "hardware-support"]
Copy link
Member

@9names 9names Dec 20, 2025

Choose a reason for hiding this comment

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

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"

Copy link
Contributor Author

@DeflateAwning DeflateAwning Dec 21, 2025

Choose a reason for hiding this comment

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

Okay, fair enough, I agree! Will remove those.


[dependencies]
cortex-m = "0.7"
Expand Down Expand Up @@ -64,3 +68,12 @@ debug = 2
debug-assertions = false
lto = 'fat'
opt-level = 3

[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 }
Comment on lines +72 to +79
Copy link
Member

@9names 9names Dec 20, 2025

Choose a reason for hiding this comment

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

Why these particular settings?
This should be discussed in the rp-rs matrix channel too.

Copy link
Contributor Author

@DeflateAwning DeflateAwning Dec 21, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ithinuel ithinuel Dec 22, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ithinuel ithinuel Dec 22, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@DeflateAwning DeflateAwning Dec 22, 2025

Choose a reason for hiding this comment

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

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)?

2 changes: 1 addition & 1 deletion build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
//! new memory settings.

use std::fs::File;
use std::io::Write;
use std::io::Write as _;
use std::path::PathBuf;

fn main() {
Expand Down
2 changes: 1 addition & 1 deletion src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#![no_std]
#![no_main]

use defmt::*;
use defmt::info;
use defmt_rtt as _;
use embedded_hal::digital::OutputPin;
use panic_probe as _;
Expand Down