Add builder pattern support for event response types #1090
Conversation
|
Hi folks, so with this PR we aim to address the issues that were introduced when we tagged events as non-exhaustive. @lmammino @jeastham1993 @ikai104 your feedback is appreciated. I looked at the examples you've posted in #1060 and added before and after in the examples folder. |
Add derived builders to events. Builders are conditionally compiled with the "builders" feature flag.
- Replace derive_builder dependency with bon v3 - Remove builder attribute configurations (bon handles automatically) - Update examples to use bon's cleaner API - Add tests for bon builder functionality - Maintain backward compatibility for builders feature Benefits: - Cleaner API: Struct::builder().build() vs StructBuilder::default().build().unwrap() - Automatic Option handling - No need for setter configurations - Better ergonomics
FullyTyped
force-pushed
the
derive-builders
branch
from
86fbf59 to
fa2af97
Compare
- Update lambda-events README with bon builder examples - Fix syntax error in comprehensive-builders example - Highlight key benefits of bon over derive_builder
- Apply automatic formatting fixes - Fix clippy suggestions
The 'sub' field represents JWT subject claims in AppSync identity contexts, not mathematical subtraction operations. This is a false positive.
Bon generates builder methods that also trigger the should_implement_trait lint
- Remove unused Builder imports - Remove .unwrap() and .map_err() calls (bon doesn't return Results) - Handle catch-all-fields feature in comprehensive example - Prefix unused variables with underscore
jlizen
left a comment
jlizen
left a comment
There was a problem hiding this comment.
The ergonomics of the new builders look good to me! Thanks for swapping out with bon, I think the compile-time safety will be well received.
My remaining nits were related to docs / examples. Up to you if you agree, fine to merge as-is.
The CI failure is due to needing the minor version bump after: #1089 (IE, separate scope)
Might be nice to fix that to get the pretty green icon, but we'll be nudged to do it by release-plz anyway.
lambda-events/README.md
Outdated
|
|
||
| ### Builder pattern support | ||
|
|
||
| The crate provides an optional `builders` feature that adds builder pattern support for event types using the [bon](https://crates.io/crates/bon) crate. This enables type-safe, immutable construction of event responses with a clean, ergonomic API. |
There was a problem hiding this comment.
Nit: bon feels like an implementation detail. We are not exposing bon specific types in our public API contract and might change what builder we use in the future (while preserving the API). Not sold on mentioning it in the README, is there anything we expect users to need to go and look up in bon docs to understand?
lambda-events/README.md
Outdated
| } | ||
| ``` | ||
|
|
||
| Key benefits of bon builders: |
There was a problem hiding this comment.
I would probably ditch this, unless we want to have some sort of INTERNALS.md to explain design decisions. It doesn't feel like it belongs in end-user docs though.
|
|
||
| #[cfg(feature = "builders")] | ||
| fn main() { | ||
| // S3 Event - Object storage notifications |
There was a problem hiding this comment.
Minor: These would be much more useful examples if they included at least one record.
| // Example showing how builders solve the Default trait requirement problem | ||
| // when using lambda_runtime with API Gateway custom authorizers | ||
| // | ||
| // ❌ OLD WAY (with Default requirement): |
There was a problem hiding this comment.
Nit: I'm not sure there is much value in showing the old way. It adds noise to this example. Shouldn't readers just be pointed to the recommended pattern without the history lesson?
There was a problem hiding this comment.
If anything, this would be a great code snippet to include in the release notes
| // Example showing how builders simplify SQS batch response construction | ||
| // when handling partial batch failures | ||
| // | ||
| // ❌ OLD WAY (with Default): |
There was a problem hiding this comment.
Same feedback on ditching old way
lambda-events/src/event/sqs/mod.rs
Outdated
|
|
||
| #[test] | ||
| #[cfg(all(feature = "sqs", feature = "builders"))] | ||
| fn test_bon_sqs_event_builder() { |
There was a problem hiding this comment.
nit: i'd probably ditch bon from the name from all of these. Someday we might want to change out the framework, and anyway we would need to maintain the api. What this is validating is the API contract, not bon specifically.
3 participants
Add derived builders to events.
Builders are conditionally compiled with the "builders" feature flag.
📬 Issue #, if available:
fixes #1060
🔏 By submitting this pull request
cargo +nightly fmt.cargo clippy --fix.