refactor(taskfile): Refactor Rust lint tasks (fixes #1451): #1472
Conversation
WalkthroughRemoved top-level Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant TaskRunner as Task runner
participant CargoFmt as cargo-workspace-fmt
participant CargoClippy as cargo-workspace-clippy
participant Toolchains as toolchains:rust
Note over TaskRunner,CargoFmt: Formatting flow (taskfiles/lint.yaml)
User->>TaskRunner: run check-rust-format / fix-rust-format
TaskRunner->>CargoFmt: invoke cargo-workspace-fmt (uses CARGO_FMT_FLAGS)
CargoFmt->>Toolchains: ensure toolchain (toolchains:rust, rust-lint-configs)
CargoFmt->>CargoFmt: run `cargo ... fmt` with {{.CARGO_FMT_FLAGS}}
CargoFmt-->>TaskRunner: exit status
TaskRunner-->>User: report result
Note over TaskRunner,CargoClippy: Linting flow (taskfiles/lint.yaml)
User->>TaskRunner: run check-rust-static / fix-rust-static
TaskRunner->>CargoClippy: invoke cargo-workspace-clippy (uses CARGO_CLIPPY_FLAGS)
CargoClippy->>Toolchains: ensure toolchain (toolchains:rust)
CargoClippy->>CargoClippy: run `cargo ... clippy` with {{.CARGO_CLIPPY_FLAGS}}
CargoClippy-->>TaskRunner: exit status
TaskRunner-->>User: report result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧠 Learnings (6)📓 Common learnings📚 Learning: 2025-10-22T21:01:31.391ZApplied to files:
📚 Learning: 2025-10-22T21:02:31.113ZApplied to files:
📚 Learning: 2025-08-25T00:45:05.464ZApplied to files:
📚 Learning: 2025-10-22T21:14:12.225ZApplied to files:
📚 Learning: 2025-10-17T19:59:35.299ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
LinZhihao-723
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
taskfile.yaml(0 hunks)taskfiles/lint.yaml(3 hunks)
💤 Files with no reviewable changes (1)
- taskfile.yaml
🔇 Additional comments (4)
taskfiles/lint.yaml (4)
802-802: Rename looks good; flags variable updated.Swapping to
cargo-workspace-fmtandCARGO_FMT_FLAGSaligns with the refactor. No issues spotted in this call site.Also applies to: 804-804
814-814: Clippy warnings gating is correct.
-- -D warningsis passed through properly. Consider adding--workspace(see below) to ensure full coverage.Also applies to: 816-816
820-820: Fix task flags are cargo‑side; OK.
--fix --allow-dirty --allow-stagedare correctly on the cargo side; no clippy args required.Also applies to: 822-822
800-823: No remnants of old task names or vars; wrapper aliases are not needed.
…/clp into rust-lint-refactor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
taskfiles/tests/main.yaml (1)
12-13: Guard sourcing$HOME/.cargo/envand verify--releaseusage
Use a file check to avoid failing when the env file is missing, and confirm that running tests with--release(which disables debug assertions) is intentional.- . "$HOME/.cargo/env" + [ -f "$HOME/.cargo/env" ] && . "$HOME/.cargo/env"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
taskfiles/tests/main.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: rust-checks (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (1)
taskfiles/tests/main.yaml (1)
5-5: Verify include path andrusttask definitionConfirm that the
toolchains: "../toolchains.yaml"entry intaskfiles/tests/main.yamlcorrectly resolves totaskfiles/toolchains.yamland that this file defines arusttask.
| tasks: | ||
| rust-all: | ||
| deps: [":toolchains:rust"] | ||
| deps: ["toolchains:rust"] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good fix on namespaced dep; consider object form for clarity.
String form works; object form reads better and avoids quoting/linters nitpicks.
- deps: ["toolchains:rust"]
+ deps:
+ - task: toolchains:rust📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deps: ["toolchains:rust"] | |
| deps: | |
| - task: toolchains:rust |
🤖 Prompt for AI Agents
In taskfiles/tests/main.yaml around line 9, the dependency is declared as a
namespaced string deps: ["toolchains:rust"]; change it to the object form (e.g.,
deps: [{ name: "toolchains", tool: "rust" }] or whatever project convention
requires) to improve readability and avoid quoting/linter issues; update the
YAML to use the agreed object keys for namespaced deps and run the linter to
confirm formatting.
There was a problem hiding this comment.
it seems a target directory is generated at the project root for build cache. shall we add the directory to .gitignore or set CARGO_TARGET_DIR to somewhere within build/?
There was a problem hiding this comment.
Sure we can set it under build
There was a problem hiding this comment.
Writing to build/rust-targets
| . "$HOME/.cargo/env" | ||
| cargo +nightly fmt --all -- {{.RUSTFMT_FLAGS}} | ||
| cargo +nightly fmt --all -- {{.CARGO_FMT_FLAGS}} |
There was a problem hiding this comment.
| . "$HOME/.cargo/env" | |
| cargo +nightly fmt --all -- {{.RUSTFMT_FLAGS}} | |
| cargo +nightly fmt --all -- {{.CARGO_FMT_FLAGS}} | |
| "$HOME/.cargo/bin/cargo" +nightly fmt --all -- {{.CARGO_FMT_FLAGS}} |
Not really important but does this work? If so and you'd like to update, there are plenty other similar cases.
There was a problem hiding this comment.
Prefer the current way as this is not just a binary call. It could set up some env variables that have side effects.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
lgtm
shall we mention that the target-dir is now changed as "build/rust-targets" in the PR description / title?
| "-Dclippy::nursery", | ||
| "-Dclippy::pedantic", | ||
| ] | ||
| target-dir = "build/rust-targets" |
There was a problem hiding this comment.
3 participants
rust-workspace-{fmt,clippy}tasks tocargo-workspace-{fmt,clippy}.RUST_{FMT,CLIPPY}parameters toCARGO_{FMT,CLIPPY}.Description
Checklist
breaking change.
Validation performed
lint:fix-rustandlint:check-rustboth worked locally.Summary by CodeRabbit
Chores
New Features
Tests