feat: add typed upcaster API#34
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe PR refactors event upcasting from raw byte transforms to typed payload transforms: EventUpcaster.transform now takes &EventRecord and returns Result; adds upcast_payload for typed decode/transform/encode; macros generate typed wrapper upcasters; tests and README examples updated to the new SourceType => TargetType syntax and chaining. ChangesTyped Event Upcasting
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/entity/upcaster.rs (1)
35-40: ⚡ Quick winExpose the inner payload error via
Error::source().Line 81 implements
std::error::Errorwithout overridingsource(), so theEventRecordErrorcaptured inPayloadTransformis not visible through the standard error chain.Suggested fix
-impl std::error::Error for UpcastError {} +impl std::error::Error for UpcastError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + UpcastError::PayloadTransform { source, .. } => Some(source), + _ => None, + } + } +}Also applies to: 81-81
🤖 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 `@src/entity/upcaster.rs` around lines 35 - 40, The Error enum's std::error::Error impl should expose the inner EventRecordError for the PayloadTransform variant via source(); update the impl of std::error::Error for Error to match other variants and return Some(&self.source) (as &dyn std::error::Error) when the variant is PayloadTransform (the variant named PayloadTransform holding source: EventRecordError), and return None for variants that have no inner error so the standard error chain reveals the EventRecordError.
🤖 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.
Inline comments:
In `@README.md`:
- Line 1379: The example call to upcast_events(events, Todo::upcasters())
ignores that upcast_events can fail; update the snippet to surface that
fallibility by handling the Result returned (e.g. propagate with ? from a
fallible function or match on Ok/Err and log/return the error). Specifically
change the use of upcast_events and the upcasted binding so errors from
upcast_events (and the Todo::upcasters() pipeline) are either returned from the
enclosing function or handled explicitly (e.g., log/convert the error), ensuring
the example compiles when copy/pasted.
In `@sourced_rust_macros/src/lib.rs`:
- Around line 204-223: generate_upcaster_tokens currently emits wrapper
functions at module scope (the wrapper iterator creating fn `#wrapper`(...) that
calls `#transform_fn`) but `#transform_fn` can be a syn::Path like Self::migrate_*
which is invalid outside an impl; move the generated wrappers inside an impl
block for the owner (wrap the wrapper_defs in impl `#owner` { ... } or emit
methods instead) so that Self::... resolves correctly, ensuring wrapper_defs
creation still references the same symbols (wrapper, transform_fn, to_version,
source_type, target_type) but are declared as impl `#owner` :: `#wrapper` or methods
inside impl `#owner` rather than free functions.
---
Nitpick comments:
In `@src/entity/upcaster.rs`:
- Around line 35-40: The Error enum's std::error::Error impl should expose the
inner EventRecordError for the PayloadTransform variant via source(); update the
impl of std::error::Error for Error to match other variants and return
Some(&self.source) (as &dyn std::error::Error) when the variant is
PayloadTransform (the variant named PayloadTransform holding source:
EventRecordError), and return None for variants that have no inner error so the
standard error chain reveals the EventRecordError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b686514-7f8a-41e6-8768-5bd8cc6fb761
📒 Files selected for processing (8)
README.mdsourced_rust_macros/src/lib.rssrc/entity/mod.rssrc/entity/upcaster.rssrc/lib.rstests/sourced_upcasting/aggregate.rstests/upcasting/aggregate.rstests/upcasting/main.rs
|
Addressed the remaining CodeRabbit nitpick in ff3312a as well: UpcastError now implements Error::source() for PayloadTransform so the wrapped EventRecordError is available through the standard error chain. Added unit coverage for that path. Validation run locally:
|
Summary
Verification
Implements [[tasks/simplify-upcaster-api]]
Summary by CodeRabbit
New Features
Documentation
Tests