Skip to content

refactor(schema): defer builder validation to build - #443

Merged
rapids-bot[bot] merged 2 commits into
rapidsai:mainfrom
johanpel:schema-yaml-cleanup
Jul 24, 2026
Merged

refactor(schema): defer builder validation to build#443
rapids-bot[bot] merged 2 commits into
rapidsai:mainfrom
johanpel:schema-yaml-cleanup

Conversation

@johanpel

Copy link
Copy Markdown
Contributor

Description

Now that a signficant portion of the YAML path has been trodden, clean some collateral slop up and solve #401 as well.

Migrates schema, FSM, and resource builders to infallible with_* setters. Builders preserve declaration order and validate duplicate or empty names during the fallible build() step.

Updates YAML lowering, code generation, and workspace callers. Removes obsolete fallible setters and unused builder APIs.

Related Issues

Closes #401.

Written by Codex.

@johanpel
johanpel marked this pull request as ready for review July 23, 2026 17:34
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The schema builder APIs now use infallible with_* chaining and defer duplicate and annotation validation to build(). FSM, resource, and YAML lowering paths propagate the revised errors and diagnostics, while tests and examples update their builder call sites.

Changes

Builder and lowering refactor

Layer / File(s) Summary
Deferred schema validation
crates/schema/src/builder/*
Schema, entity, event, record, and annotation builders now use vector-backed storage, infallible with_* methods, and fallible validation during build().
FSM and resource build behavior
crates/fsm/src/builder.rs, crates/resource/src/builder.rs
FSM construction adopts the revised builders and returns the first topology error; resource capacity validation moves to build() and aggregated resource errors are removed.
Diagnostic-aware YAML lowering
crates/yaml/src/lib.rs, crates/yaml/src/lower.rs, crates/yaml/tests/diagnostics.rs
YAML lowering returns Option<Schema>, converts builder failures into diagnostics, and stops parsing when schema construction fails.
Updated builder call sites
crates/constraints/*, crates/fsm/tests/*, crates/instrumentation-build/*, crates/ref-*/tests/*, crates/resource/tests/*, crates/schema/src/test_utils.rs, crates/schema/src/visitor.rs
Tests, examples, and helper constructors switch from try_* methods to with_* chaining with final build-result handling.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

Suggested reviewers: mbrobbel, dhruv9vats

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: moving builder validation to the build step.
Description check ✅ Passed The description covers the refactor and linked issue, but omits the Testing section and Screenshots section from the template.
Linked Issues check ✅ Passed The changes implement #401 by making schema-related builder setters infallible and deferring validation to build().
Out of Scope Changes check ✅ Passed The YAML, codegen, and caller updates are part of the stated refactor scope, with no clear unrelated changes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

coderabbitai[bot]

This comment was marked as resolved.

@coderabbitai coderabbitai 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.

🧹 Nitpick comments (1)
crates/schema/src/builder/entity.rs (1)

59-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add regression tests for deferred event validation.

Please cover duplicate event names being rejected by build() and distinct events retaining declaration order. The shown test only exercises the empty-events case, leaving the primary behavior changed here unverified.

🤖 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/schema/src/builder/entity.rs` around lines 59 - 72, Add regression
tests for EntityBuilder::build covering duplicate event names returning the
expected BuilderError and distinct events preserving their declaration order in
the resulting Entity. Keep the existing empty-events test, and construct events
through the same test helpers or public APIs already used in the entity builder
tests.
🤖 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/schema/src/builder/entity.rs`:
- Around line 59-72: Add regression tests for EntityBuilder::build covering
duplicate event names returning the expected BuilderError and distinct events
preserving their declaration order in the resulting Entity. Keep the existing
empty-events test, and construct events through the same test helpers or public
APIs already used in the entity builder tests.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: dcad4fd0-7890-4a88-bbfb-96b122a35bd3

📥 Commits

Reviewing files that changed from the base of the PR and between ead6d6d and 5f40c82.

📒 Files selected for processing (1)
  • crates/schema/src/builder/entity.rs

@johanpel

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot
rapids-bot Bot merged commit 2f7aedb into rapidsai:main Jul 24, 2026
11 checks passed
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.

Make schema builder setters infallible

2 participants