Skip to content

refactor(shared): adopt style guide rules around pub/module visibility - #4664

Merged
chet merged 1 commit into
NVIDIA:mainfrom
chet:gh-issue-4557
Aug 6, 2026
Merged

refactor(shared): adopt style guide rules around pub/module visibility#4664
chet merged 1 commit into
NVIDIA:mainfrom
chet:gh-issue-4557

Conversation

@chet

@chet chet commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

This adopts the new style guide rules around module visibility introduced in #4522, applying the correct visibility throughout the remaining shared model and tooling crates.

Primary callouts are:

  • Resolve all 47 compiler-proven findings across xtask, api-model, uuid, api-db, libmlx, mqttea, libnmxm, libnmxc, and api with private or pub(super) visibility based on actual callers.
  • Keep API/model DTO fields, intentional re-exports, UUID macros, and test-support surfaces public where downstream crates still use them.
  • Scope SQL row helpers, SLA constants, UUID wire adapters, xtask command internals, library implementation types, and test fixtures to the modules that use them.
  • Make the Switch and PowerShelf SLA modules private after direct caller review, along with the remaining private fields in the MQTT test messages.
  • Leave generated source, serialized data, wire contracts, and runtime behavior unchanged.

Tests pass!

Related issues

This supports #4557.

This supports the updated Rust visibility scoping guidelines in STYLE_GUIDE.md, established in #4522.

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

The targeted unit, integration, and doc-test suites for all nine crates passed, including a post-review rerun for api-model and mqttea. I also ran:

make core/tests TEST_ARGS="-p xtask -p carbide-api-model -p carbide-uuid -p carbide-api-db -p carbide-libmlx -p mqttea -p libnmxm -p libnmxc -p carbide-api --all-features"
cargo test --locked -p carbide-api-model -p mqttea --all-features
cargo make format-nightly
cargo make clippy
cargo make carbide-lints
cargo clippy --locked -p xtask -p carbide-api-model -p carbide-uuid -p carbide-api-db -p carbide-libmlx -p mqttea -p libnmxm -p libnmxc -p carbide-api --all-targets --all-features --message-format short -- --force-warn unreachable-pub
git diff --check

Additional Notes

The current main baseline identified 47 overbroad declarations across these nine crates. The final 31-file diff also narrows adjacent fields and module boundaries with direct caller evidence. No unreachable_pub warning remains in the owned source trees.

Public API/model DTO fields, intentional re-exports, UUID/version macros, generated source, and test support with downstream consumers remain unchanged. This does not change serialized data, wire contracts, or runtime behavior.

Local CodeRabbit returned zero findings. The independent read-only Claude review found two applicable follow-through items around private MQTT test fields and the Switch/PowerShelf SLA module boundaries; both are incorporated.

The review also proposed spelling the crate-root logging child boundary as pub(crate) instead of pub(super). That stays pub(super) because the STYLE_GUIDE.md caller table maps parent-module callers and descendants to pub(super); although both spellings reach the same callers for a crate-root child, pub(super) records the actual relationship.

Closes #4557

This adopts the new style guide rules around module visibility introduced in NVIDIA#4522, applying the correct visibility throughout the remaining shared model and tooling crates.

Primary callouts are:
- Resolve all 47 compiler-proven findings across `xtask`, `api-model`, `uuid`, `api-db`, `libmlx`, `mqttea`, `libnmxm`, `libnmxc`, and `api` with private or `pub(super)` visibility based on actual callers.
- Keep API/model DTO fields, intentional re-exports, UUID macros, and test-support surfaces public where downstream crates still use them.
- Scope SQL row helpers, SLA constants, UUID wire adapters, xtask command internals, library implementation types, and test fixtures to the modules that use them.
- Make the Switch and PowerShelf SLA modules private after direct caller review, along with the remaining private fields in the MQTT test messages.
- Leave generated source, serialized data, wire contracts, and runtime behavior unchanged.

Tests pass!

This supports NVIDIA#4557.

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>
@chet
chet requested a review from a team as a code owner August 6, 2026 08:54
@chet

chet commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full_review, thanks!

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d9e1f22c-3463-45aa-bc64-88e1a10dd5a8

📥 Commits

Reviewing files that changed from the base of the PR and between 4f0e11f and b16e053.

📒 Files selected for processing (31)
  • crates/api-db/src/explored_endpoints.rs
  • crates/api-db/src/explored_managed_host.rs
  • crates/api-db/src/ib_partition.rs
  • crates/api-db/src/measured_boot/report.rs
  • crates/api-model/src/dpa_interface/slas.rs
  • crates/api-model/src/ib_partition/slas.rs
  • crates/api-model/src/lib.rs
  • crates/api-model/src/network_segment/slas.rs
  • crates/api-model/src/power_shelf/mod.rs
  • crates/api-model/src/power_shelf/slas.rs
  • crates/api-model/src/site_explorer/mod.rs
  • crates/api-model/src/switch/mod.rs
  • crates/api-model/src/switch/slas.rs
  • crates/api/src/logging.rs
  • crates/libmlx/tests/runner/common/mod.rs
  • crates/libnmxc/src/nmxc_api.rs
  • crates/libnmxm/src/nmxm_api.rs
  • crates/mqttea/tests/client.rs
  • crates/mqttea/tests/registry.rs
  • crates/uuid/src/device/mod.rs
  • crates/uuid/src/machine/mod.rs
  • crates/uuid/src/power_shelf/mod.rs
  • crates/uuid/src/rack/mod.rs
  • crates/uuid/src/switch/mod.rs
  • crates/uuid/src/typed_uuids.rs
  • crates/xtask/src/error_message_case.rs
  • crates/xtask/src/event_names.rs
  • crates/xtask/src/isolated_package_builds.rs
  • crates/xtask/src/metric_docs.rs
  • crates/xtask/src/squash_migrations.rs
  • crates/xtask/src/workspace_deps.rs

Summary by CodeRabbit

  • Refactor
    • Restricted internal APIs, helpers, compatibility types, and SLA definitions to their appropriate module scopes.
    • Reduced unnecessary public exposure without changing runtime behavior, data formats, or SLA values.
    • Kept test-only utilities and protobuf compatibility structures internal where they are not intended for external use.

Walkthrough

The pull request narrows Rust visibility across internal database APIs, model constants, library wrappers, protobuf compatibility types, test fixtures, and xtask interfaces. Signatures, data shapes, values, and runtime behavior remain unchanged.

Changes

Visibility narrowing

Layer / File(s) Summary
Database internal declarations
crates/api-db/src/*
Database row-mapping structs and JournalData::new_from_values now use restricted visibility.
Model modules and SLA contracts
crates/api-model/src/*
SLA modules, constants, serialization helpers, and a test helper now use pub(super) or private visibility.
API wrappers and test helpers
crates/api/src/logging.rs, crates/libnmxc/src/*, crates/libnmxm/src/*, crates/libmlx/tests/runner/common/*
Logging setup, API wrappers, constructors, fields, and shared test helpers are no longer publicly exposed.
Compatibility types and test fixtures
crates/uuid/src/*, crates/mqttea/tests/*
Protobuf compatibility shims and test fixture types retain their data shapes while using restricted visibility.
Xtask checker interfaces
crates/xtask/src/*
Checker functions, result types, command arguments, and reporting methods now use pub(super) visibility.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the visibility refactor and follows concise, specific wording.
Description check ✅ Passed The description directly explains the visibility changes, scope, preserved behavior, testing, and related objectives.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@chet: I will perform a complete review of PR #4664.

🐇

✅ Action performed

Full review finished.

@chet
chet merged commit 5141e4a into NVIDIA:main Aug 6, 2026
64 checks passed
@chet
chet deleted the gh-issue-4557 branch August 6, 2026 16:40
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.

Tighten Remaining Shared Library Visibility

2 participants