Skip to content

[improve][ci] Check formatting in CI - #150

Merged
thep2p merged 3 commits into
mainfrom
thep2p/ci-fmt-check
Oct 1, 2026
Merged

thep2p merged 3 commits into
mainfrom
thep2p/ci-fmt-check

Conversation

@thep2p

@thep2p thep2p commented Sep 29, 2026

Copy link
Copy Markdown
Owner

No description provided.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

No unresolved review issues were identified.

Review effort: Lite
Findings: None

What changed in this PR

Adds Rust formatting validation to the CI lint job.

Changes:

  • Runs make format during CI.
File Description
.github/​workflows/​rust.yml Adds the formatting validation step.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@thep2p

thep2p commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

@claude review

@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Claude finished @thep2p's task —— View job


Code Review: PR #150 — Check formatting in CI

  • Read changed files
  • Analyze CI workflow change
  • Analyze base_node.rs change
  • Post review

Verdict: Approve ✅ — small, correct, and well-scoped change.

What this does

  • Adds a Check Format step to the lint job in .github/workflows/rust.yml:36-37, running make format.
  • make format maps to cargo fmt --all -- --check (Makefile:25-28), which is the right choice for CI: it's non-mutating and exits non-zero on unformatted code, so a violation fails the build instead of silently rewriting files.
  • The src/node/base_node.rs:2857-2859 change collapses a multi-line assert_eq!(dest, s_id, ...) onto one line — exactly the kind of fix this new check now enforces, so it's self-consistent with the PR's purpose.

Observations (all non-blocking)

  • Step ordering: Check Format sits between Install Tools and Run Lint. Placing the format check before lint is reasonable (fast, cheap failure). No change needed.
  • Pre-existing, out of scope: Makefile:1 and :7 use .PHONEY (typo for .PHONY), so those targets aren't actually declared phony. Harmless here but worth a follow-up cleanup — not something to fix in this PR.
  • Pre-existing, out of scope: actions-rs/toolchain@v1 is unmaintained; a future migration to dtolnay/rust-toolchain would be worth considering, but it's unrelated to this change.

No bugs, security, or correctness concerns. LGTM.
• thep2p/ci-fmt-check

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Small, correct, and well-scoped. 'make format' runs 'cargo fmt --all -- --check', which is the right non-mutating choice for CI (fails on unformatted code rather than rewriting). The base_node.rs change is a self-consistent fmt fix. Non-blocking nits: pre-existing .PHONEY typos in the Makefile and the unmaintained actions-rs/toolchain@v1 action, both out of scope here. LGTM.

@thep2p
thep2p merged commit 78e784a into main Oct 1, 2026
5 checks passed
@thep2p
thep2p deleted the thep2p/ci-fmt-check branch October 1, 2026 00:10
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.

2 participants