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

Fix all clippy lints #714

Merged
merged 5 commits into from
Jan 11, 2024
Merged

Fix all clippy lints #714

merged 5 commits into from
Jan 11, 2024

Conversation

jbeaurivage
Copy link
Contributor

@jbeaurivage jbeaurivage commented Jan 8, 2024

Summary

Fix all HAL clippy lints.

Also up for debate: I've switched the CI build step to clippy, and having it deny all clippy warnings. However some might think that it is too aggressive.

Closes #574.

Checklist

  • CHANGELOG.md for the BSP or HAL updated
  • All new or modified code is well documented, especially public items
  • No new warnings or clippy suggestions have been introduced (see CI or check locally)

@ianrrees
Copy link
Contributor

ianrrees commented Jan 8, 2024

I like the idea of requiring a clean Clippy pass in CI - it seems like overall there's less headache in preventing the warnings at the outset, vs everyone ignoring them in later development

@jbeaurivage
Copy link
Contributor Author

Sounds good. That doesn't mean we should blindly follow clippy's advice all the time; #[allow]ing certain lints is totally acceptable, but I think we should try to explain why we're allowing it with a comment where reasonable.

@ianrrees
Copy link
Contributor

Showing my ignorance here: does clippy also build the code? Just wondering if there might be some kinds of issue that would pass cargo clippy but not cargo build - if so we might want CI to do both

@jbeaurivage
Copy link
Contributor Author

@ianrrees you're right, it doesn't build the code - sometimes things like linker errors will pop up when building but not checking. However, since we do build the BSP examples, I'm fairly confident these issues would be caught by CI regardless.

@ianrrees
Copy link
Contributor

since we do build the BSP examples, I'm fairly confident these issues would be caught by CI regardless.

Yep, that makes a lot of sense to me! Any issues that "escaped" would be good to cover with additional examples - sortof a build time regression test suite.

@ianrrees ianrrees merged commit 40680e7 into atsamd-rs:master Jan 11, 2024
107 checks passed
@jbeaurivage jbeaurivage deleted the clippy branch January 11, 2024 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider running clippy more aggressively
2 participants