-
Notifications
You must be signed in to change notification settings - Fork 0
Document async-stream as canonical Response::Stream helper #154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6e95b39
c6add81
13fbe26
6223a3b
3b1fd3c
b9dc6fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,8 +29,10 @@ design documents. | |
| - [x] **Leak-proof `SessionRegistry`** using `dashmap::DashMap` and `Weak` | ||
| pointers ([Design §4.2][design-registry], | ||
| [Resilience Guide §3.2][resilience-registry]). | ||
| - [ ] **Document `async-stream`** for creating `Response::Stream` values | ||
| - [x] **Document `async-stream`** for creating `Response::Stream` values | ||
| ([Roadmap #2.4][roadmap-2-4]). | ||
| - [ ] **Example handler using `async-stream`** demonstrating `Response::Stream` | ||
| generation in the examples directory. | ||
|
Comment on lines
+32
to
+35
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Bullet exceeds 80-column guideline and should be wrapped The added bullet for the example handler is > 100 characters, violating the - - [ ] **Example handler using `async-stream`** demonstrating `Response::Stream` generation in the examples directory.
+ - [ ] **Example handler using `async-stream`** demonstrating `Response::Stream`
+ generation in the *examples* directory.
🤖 Prompt for AI Agents |
||
| - [ ] **Tests covering streams and push delivery** drawing on | ||
| [Testing Guide §4][testing-guide-advanced]. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,11 +38,10 @@ The implementation must satisfy the following core requirements: | |
| ## 3. Core Architecture: Declarative Streaming | ||
|
|
||
| The cornerstone of this design is a move to a purely **declarative streaming | ||
| model**. Instead of providing handlers with an imperative `FrameSink` to push | ||
| frames into, handlers will declaratively return a description of the entire | ||
| response. This approach significantly simplifies the API surface, improves | ||
| testability, and eliminates a class of resource management issues associated | ||
| with sink-based designs. | ||
| model**. Handlers declaratively return a description of the entire response. | ||
| This approach significantly simplifies the API surface, improves testability, | ||
| and eliminates a class of resource management issues associated with sink-based | ||
| designs. | ||
|
|
||
| ### 3.1 The Connection Actor's Role | ||
|
|
||
|
|
@@ -61,12 +60,12 @@ explicit channel management. | |
| ### 3.2 The `async-stream` Crate | ||
|
|
||
| To provide an ergonomic way for developers to generate streams using | ||
| imperative-style logic (e.g., inside a `for` loop), `wireframe` will adopt and | ||
| recommend the `async-stream` crate. This crate provides macros (`stream!` and | ||
| imperative-style logic (e.g., inside a `for` loop), `wireframe` adopts and | ||
| recommends the `async-stream` crate. This crate provides macros (`stream!` and | ||
| `try_stream!`) that transform imperative `yield` statements into a fully | ||
| compliant `Stream` object. This gives developers the best of both worlds: the | ||
| intuitive feel of imperative code generation without the API complexity of a | ||
| separate `FrameSink` type. | ||
| compliant `Stream` object. This gives developers the intuitive feel of | ||
| imperative code generation with minimal API complexity. The library recommends | ||
| this pattern as the canonical way to build `Response::Stream` values. | ||
|
Comment on lines
+63
to
+68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Minor line-length breach – wrap for consistency Lines 63-66 run a little past 80 columns. While tables are exempt, regular 🤖 Prompt for AI Agents |
||
|
|
||
| ## 4. Public API Surface | ||
|
|
||
|
|
@@ -252,7 +251,7 @@ hang. | |
| | Category | Objective | Success Metric | | ||
| | API Correctness | The Response enum and FrameStream type alias are implemented exactly as specified in this document. | 100% of the public API surface is present and correctly typed. | | ||
| | Functionality | A handler returning a stream of N frames results in N frames being written to the socket in the correct order. | A test suite confirms 100% frame delivery and strict ordering for Response::Vec and Response::Stream. | | ||
| | Ergonomics | The async-stream pattern is documented as the canonical approach for dynamic stream generation and is demonstrably simpler than a FrameSink API. | The official examples and documentation exclusively use the declarative Response model. | | ||
| | Ergonomics | The async-stream pattern is documented as the canonical approach for dynamic stream generation. | The official examples and documentation exclusively use the declarative Response model. | | ||
| | Performance | The Response::Vec variant has measurably lower allocation and dispatch overhead than Response::Stream for small, fixed-size responses. | A criterion benchmark confirms that Response::Vec is at least 50% faster and performs fewer allocations than Response::Stream for a response of 10 frames. | | ||
| | Error Handling | A WireframeError::Protocol error yielded from a stream correctly triggers the handle_error protocol callback without terminating the connection. | An integration test confirms that a protocol-level error is correctly formatted and sent to the client, while the connection remains open. | | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Table rows exceed 80 columns
The requirement rows G2–G5 are ~130 chars each. To satisfy MD013 you can either
soft-wrap the cell contents or silence the rule for those lines with
<!-- markdownlint-disable-line MD013 -->.🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Table rows are allowed to exceed 80 columns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leynos, understood! I've noted that table rows are exempt from the 80-column wrapping requirement for markdown files.
✏️ Learnings added