refactor(generated): adopt style guide rules for pub/module visibility - #4682
Conversation
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
Summary by CodeRabbit
WalkthroughThe PR makes one internal conversion method private and adds documented ChangesVisibility and lint cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/ok to test 10d25be |
|
/ok to test 10d25be |
|
/ok to test 10d25be |
1 similar comment
|
/ok to test 10d25be |
The final visibility audit found 1,729 declarations that need to stay public even though Rust callers cannot reach them through the module tree: generated tonic, Kopium, prost, and Askama items plus the Kea C++ ABI. So, this keeps reasoned `unreachable_pub` allowances at the handwritten module and `include!` boundaries that own those declarations. Generated output stays unchanged, and the lint remains active everywhere outside those reviewed boundaries. Primary callouts are: - Contain generated tonic/prost and Kopium declarations at the handwritten module or `include!` boundary that owns each output. - Narrow the SSH mock's handwritten generated-module index before allowing the generated child modules. - Contain Askama's generated filter helpers inside the four modules where `#[askama::filter_fn]` expands them. - Preserve 55 cbindgen and dynamically loaded Kea symbols at six module-scoped C/C++ ABI boundaries. - Make the one API Core helper added after its crate cleanup private. - Leave runtime behavior, wire contracts, templates, and generated source unchanged. This supports NVIDIA#4547. This supports the updated Rust visibility scoping guidelines in `STYLE_GUIDE.md`, established in NVIDIA#4522. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
🐇✅ ✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4682.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/machine-controller/src/rpc.rs (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the blanket
dead_codeallowance.
#![allow(dead_code)]suppresses dead-code diagnostics for every item emitted byscout_firmware_upgrade. Remove it, or gate test- and feature-specific generated items with#[cfg(...)]and fix the generator output for production items. Keep the scopedunreachable_puballowance for the RPC boundary.Proposed change
pub(crate) mod scout_firmware_upgrade { - #![allow(dead_code)] #![allow( unreachable_pub, reason = "tonic_prost_build emits public items for this crate-internal protocol module"As per coding guidelines: avoid
#[allow(...)], especially#[allow(dead_code)]; fix the issue or use#[cfg(test)]/feature gating where appropriate. As per path instructions: prefer clippy-clean Rust changes without broad allows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/machine-controller/src/rpc.rs` at line 19, Remove the module-level #![allow(dead_code)] from the RPC module. Ensure test- or feature-specific generated items are gated with the appropriate #[cfg(...)] conditions and production items are emitted and referenced correctly, while preserving the scoped unreachable_pub allowance for the RPC boundary.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/machine-controller/src/rpc.rs`:
- Line 19: Remove the module-level #![allow(dead_code)] from the RPC module.
Ensure test- or feature-specific generated items are gated with the appropriate
#[cfg(...)] conditions and production items are emitted and referenced
correctly, while preserving the scoped unreachable_pub allowance for the RPC
boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 42fc6ef0-6ed2-413e-b0c3-9a15b943a4f8
⛔ Files ignored due to path filters (1)
crates/ssh-console-mock-api-server/src/generated/mod.rsis excluded by!**/generated/**,!crates/ssh-console-mock-api-server/src/generated/**
📒 Files selected for processing (12)
crates/agent/src/dhcp_server_grpc_client.rscrates/api-core/src/admission/engine.rscrates/api-web/src/filters.rscrates/api-web/src/ipxe_template.rscrates/api-web/src/redfish_actions.rscrates/api-web/src/resource_pool.rscrates/dhcp-server/src/grpc_server.rscrates/dhcp/src/kea/mod.rscrates/dhcp/src/lib.rscrates/dpf/src/crds/mod.rscrates/health/src/collectors/nvue/gnmi/mod.rscrates/machine-controller/src/rpc.rs
This adopts the new style guide rules around
puband module visibility introduced in #4522, defining the intentional generated and non-Rust boundaries that remain after the crate cleanup passes.Primary callouts are:
unreachable_puballowances at the source-owned module orinclude!boundary that owns each output.#[askama::filter_fn]expands them.Related issues
This supports #4547.
This supports the updated Rust visibility scoping guidelines in
STYLE_GUIDE.md, established in #4522.Type of Change
Breaking Changes
Testing
The focused test runs covered 949 existing tests across the affected crates. The six packages touched by the final review adjustment were rerun afterward.
Additional Notes
The raw forced-lint inventory contains 1,729 intentional declarations: 1,400 in the SSH mock's generated children, 252 in
OUT_DIRor Kopium output, 22 emitted by Askama, and 55 at the Kea ABI. The normal workspace lint and the strict-D unreachable-pubrun both complete with no diagnostics outside the reviewed boundaries.make core/testscould not start the 32 Machine Controller SQLx integration cases because the local Docker daemon is unavailable. The Machine Controller library tests and the full all-target/all-feature Clippy build passed locally; CI retains coverage of the Docker-backed path.The tracked generated outputs and cbindgen header retained their pre-change hashes.
Closes #4547