-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add Rust pre-commit hooks for desktop native #17823
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
base: main
Are you sure you want to change the base?
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
This reverts commit 6b76467.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #17823 +/- ##
==========================================
- Coverage 41.83% 41.82% -0.01%
==========================================
Files 3588 3588
Lines 104118 104118
Branches 15698 15698
==========================================
- Hits 43557 43550 -7
- Misses 58717 58724 +7
Partials 1844 1844 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| "cargo +nightly -Z unstable-options -C ./desktop_native fmt", | ||
| "cargo +nightly -Z unstable-options -C ./desktop_native clippy --all-features --all-targets --tests -- -D warnings", | ||
| ], | ||
| "*Cargo.toml": (stagedFiles) => [ | ||
| "cargo +nightly -Z unstable-options -C ./desktop_native udeps --workspace --all-features --all-targets", | ||
| "cargo +nightly -Z unstable-options -C ./desktop_native sort --workspace --check", |
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.
unstable-options required for -C
|
Claude finished @neuronull's task —— View job Code Review CompleteTasks
Overall Assessment: REQUEST CHANGESThis PR adds valuable pre-commit hooks for Rust code, but the implementation has several critical issues that need to be addressed before merge. Critical Issues1. Pre-commit hooks ignore staged files parameter ❌Location: The "*.rs": (stagedFiles) => [ // stagedFiles is never used!
"cargo +nightly -Z unstable-options -C ./desktop_native fmt",
"cargo +nightly -Z unstable-options -C ./desktop_native clippy --all-features --all-targets --tests -- -D warnings",
],Impact:
Recommendation: Remove the unused parameter and add a comment explaining the workspace-wide behavior: export default {
// Note: These run on the entire desktop_native workspace regardless of which files are staged.
// This matches CI behavior and ensures workspace consistency.
"*.rs": () => [
"cargo +nightly -Z unstable-options -C ./desktop_native fmt --check",
"cargo +nightly -Z unstable-options -C ./desktop_native clippy --all-features --all-targets --tests -- -D warnings",
],
"*Cargo.toml": () => [
"cargo +nightly -Z unstable-options -C ./desktop_native sort --workspace --check",
],
};2. Missing --check flag on cargo fmt
|
|
resolved a merge conflict |

🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-29177
📔 Objective
As we recently added the udeps and cargo sort checks to CI and already had clippy and fmt checks, the need for pre commit checks is increasing.
This PR adds pre-commit checks the same ones we have in CI for the Rust bits in desktop native.
The changes to the version of
cargo-sortin CI was done to get to a version that supports the workspace.dependencies and more sections of the toml file in general.The changes to the individual Cargo.toml files were to update them with cargo-sort
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes