feat(resource): support unit resources - #429
Conversation
📝 WalkthroughWalkthroughChangesUnit resource support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
crates/resource/src/lib.rs-34-49 (1)
34-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify that unit resources are exempt from the bounds-event requirement.
Requirement 2 says every bounded resource capacity requires a bounds event, but the unit-resource section defines an implicit capacity with bound 1 while the builder intentionally emits no bounds record. Say “declared capacity” and explicitly exclude the implicit unit capacity.
Suggested wording
-/// 2. If and only if any of the resource's capacities have a bound, the -/// resource entity has at least one event (the "bounds event") which -/// declares the bounds of all capacities that are bounded. +/// 2. If and only if any declared capacity has a bound, the resource entity +/// has at least one event (the "bounds event") which declares the bounds +/// of all bounded declared capacities. The implicit capacity of a unit +/// resource is not represented by a bounds event.🤖 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/resource/src/lib.rs` around lines 34 - 49, Update the resource invariants documentation in lib.rs so requirement 2 refers to “declared” capacities and explicitly excludes the implicit capacity of unit resources from the bounds-event requirement. Keep the Unit resources section consistent with this clarification and do not change builder behavior.
🧹 Nitpick comments (1)
crates/resource/tests/resource-constraint.rs (1)
126-131: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover unit-resource usage, not only the empty definition.
This test proves that a capacity-less definition is accepted, but not that an FSM can reference the generated fieldless usage record (
ThreadUsage {}). Add coverage for accepted FSM usage, rejected non-FSM usage, and rejection of a unit usage containing an explicit field.🤖 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/resource/tests/resource-constraint.rs` around lines 126 - 131, Extend unit_resource_is_accepted to test usage behavior for the capacity-less Thread resource: accept an FSM referencing the generated fieldless ThreadUsage record, reject usage from a non-FSM context, and reject ThreadUsage when it contains an explicit field. Reuse the existing schema, resource_entity, and resource_errors helpers and assert the corresponding valid or invalid results.
🤖 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.
Other comments:
In `@crates/resource/src/lib.rs`:
- Around line 34-49: Update the resource invariants documentation in lib.rs so
requirement 2 refers to “declared” capacities and explicitly excludes the
implicit capacity of unit resources from the bounds-event requirement. Keep the
Unit resources section consistent with this clarification and do not change
builder behavior.
---
Nitpick comments:
In `@crates/resource/tests/resource-constraint.rs`:
- Around line 126-131: Extend unit_resource_is_accepted to test usage behavior
for the capacity-less Thread resource: accept an FSM referencing the generated
fieldless ThreadUsage record, reject usage from a non-FSM context, and reject
ThreadUsage when it contains an explicit field. Reuse the existing schema,
resource_entity, and resource_errors helpers and assert the corresponding valid
or invalid results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 6984cead-e49d-4f93-b734-47fbbacbaf56
📒 Files selected for processing (3)
crates/resource/src/builder.rscrates/resource/src/lib.rscrates/resource/tests/resource-constraint.rs
|
/merge |
…430) Generate fieldless records as Rust unit structs. See #429 for an example of why this is nicer. Authors: - Johan Peltenburg (https://github.com/johanpel) Approvers: - Matthijs Brobbel (https://github.com/mbrobbel) URL: #430
Modify the requirements of the resource constraint such that "unit resources" can be expressed (a concept already present in PoC Quent).
Unit resources are a special case of a resource that can only be used by one thing at a time. This is useful to represent e.g. thread resources. Technically speaking, they have an implicit unnamed dimensionless capacity with the fixed bound 1.
Note that by allowing them to have no explicit capacities, the usage record simply becomes an empty record, so in instrumentation this will look something like this:
I will follow up to remove the
{}.We could consider further sugaring this by removing the need to set ref data later, but I kind of like that this syntax explicitly exposes its a usage of a thread resource within the context of Quent's resource concept.