Skip to content

refactor(taskfile): Refactor Rust lint tasks (fixes #1451): #1472

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
wants to merge 11 commits into
base: main
Choose a base branch
from
+31 −21
Open

refactor(taskfile): Refactor Rust lint tasks (fixes #1451): #1472

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c4d8d8c
Refactor.
LinZhihao-723 Oct 22, 2025
47c4924
Merge branch 'main' into rust-lint-refactor
LinZhihao-723 Oct 22, 2025
2fa23eb
Fix...
LinZhihao-723 Oct 22, 2025
6b17825
Merge branch 'rust-lint-refactor' of https://github.com/LinZhihao-723…
LinZhihao-723 Oct 22, 2025
799a015
Merge branch 'main' into rust-lint-refactor
junhaoliao Oct 23, 2025
f432e1e
Merge branch 'main' into rust-lint-refactor
LinZhihao-723 Oct 23, 2025
87c722b
Merge branch 'main' into rust-lint-refactor
LinZhihao-723 Oct 28, 2025
becbc71
Apply suggestions from code review
LinZhihao-723 Nov 10, 2025
c3ca815
Apply code review comments.
LinZhihao-723 Nov 10, 2025
1fea11a
Merge branch 'oss-main' into rust-lint-refactor
LinZhihao-723 Nov 10, 2025
d532f48
Done.
LinZhihao-723 Nov 10, 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
1 change: 0 additions & 1 deletion taskfile.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ includes:
docs: "taskfiles/docs.yaml"
lint: "taskfiles/lint.yaml"
tests: "taskfiles/tests/main.yaml"
toolchains: "taskfiles/toolchains.yaml"
utils: "tools/yscope-dev-utils/exports/taskfiles/utils/utils.yaml"

vars:
Expand Down
45 changes: 27 additions & 18 deletions taskfiles/lint.yaml
View file
Open in desktop
Copy link
Member

@junhaoliao junhaoliao Oct 25, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@LinZhihao-723 LinZhihao-723 Oct 28, 2025

Choose a reason for hiding this comment

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

Sure we can set it under build

Copy link
Member Author

@LinZhihao-723 LinZhihao-723 Nov 10, 2025

Choose a reason for hiding this comment

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

Writing to build/rust-targets

Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
version: "3"

includes:
toolchains: "./toolchains.yaml"

vars:
# Ignore https://github.com/msgpack/msgpack-c/issues/1098
G_CLANG_TIDY_LINE_FILTER_IGNORE_MSGPACK_1098: >-
Expand Down Expand Up @@ -796,27 +799,27 @@ tasks:

check-rust-format:
cmds:
- task: "rust-workspace-fmt"
- task: "cargo-workspace-fmt"
vars:
RUSTFMT_FLAGS: "--check"
CARGO_FMT_FLAGS: "--check"

fix-rust-format:
cmds:
- task: "rust-workspace-fmt"
- task: "cargo-workspace-fmt"
vars:
RUSTFMT_FLAGS: ""
CARGO_FMT_FLAGS: ""

check-rust-static:
cmds:
- task: "rust-workspace-clippy"
- task: "cargo-workspace-clippy"
vars:
RUST_CLIPPY_FLAGS: "-- -D warnings"
CARGO_CLIPPY_FLAGS: "-- -D warnings"

fix-rust-static:
cmds:
- task: "rust-workspace-clippy"
- task: "cargo-workspace-clippy"
vars:
RUST_CLIPPY_FLAGS: "--fix --allow-dirty --allow-staged"
CARGO_CLIPPY_FLAGS: "--fix --allow-dirty --allow-staged"

rust-lint-configs:
internal: true
Expand All @@ -826,25 +829,31 @@ tasks:
{{.ROOT_DIR}}/tools/yscope-dev-utils/exports/lint-configs/.rustfmt.toml \
{{.ROOT_DIR}}/.rustfmt.toml

rust-workspace-fmt:
# Runs `cargo +nightly fmt` in the root Rust workspace directory with the specified flags.
#
# @param {string} CARGO_FMT_FLAGS The flags to pass to the `cargo +nightly fmt` command.
cargo-workspace-fmt:
internal: true
deps: ["rust-lint-configs", ":toolchains:rust"]
dir: "{{.ROOT_DIR}}"
requires:
vars: ["RUSTFMT_FLAGS"]
vars: ["CARGO_FMT_FLAGS"]
dir: "{{.ROOT_DIR}}"
deps: ["rust-lint-configs", "toolchains:rust"]
cmd: |-
. "$HOME/.cargo/env"
cargo +nightly fmt --all -- {{.RUSTFMT_FLAGS}}
cargo +nightly fmt --all -- {{.CARGO_FMT_FLAGS}}
Comment on lines 842 to +843
Copy link
Contributor

@Bill-hbrhbr Bill-hbrhbr Oct 28, 2025

Choose a reason for hiding this comment

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

Suggested change
. "$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.

Copy link
Member Author

@LinZhihao-723 LinZhihao-723 Nov 10, 2025

Choose a reason for hiding this comment

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

Prefer the current way as this is not just a binary call. It could set up some env variables that have side effects.


rust-workspace-clippy:
# Runs `cargo +nightly clippy` in the root Rust workspace directory with the specified flags.
#
# @param {string} CARGO_CLIPPY_FLAGS The flags to pass to the `cargo +nightly clippy` command.
cargo-workspace-clippy:
internal: true
deps: [":toolchains:rust"]
dir: "{{.ROOT_DIR}}"
requires:
vars: ["RUST_CLIPPY_FLAGS"]
vars: ["CARGO_CLIPPY_FLAGS"]
dir: "{{.ROOT_DIR}}"
deps: ["toolchains:rust"]
cmd: |-
. "$HOME/.cargo/env"
cargo +nightly clippy --all-targets --all-features {{.RUST_CLIPPY_FLAGS}}
cargo +nightly clippy --all-targets --all-features {{.CARGO_CLIPPY_FLAGS}}

venv:
internal: true
Expand Down
3 changes: 2 additions & 1 deletion taskfiles/tests/main.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@ version: "3"

includes:
integration: "integration.yaml"
toolchains: "../toolchains.yaml"

tasks:
rust-all:
deps: [":toolchains:rust"]
deps: ["toolchains:rust"]
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 22, 2025

Choose a reason for hiding this comment

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

🧹 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.

Suggested change
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.

dir: "{{.ROOT_DIR}}"
cmd: |-
. "$HOME/.cargo/env"
Expand Down
Loading