Implement trait foundations #8
Conversation
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis update introduces new modules for extractors, frame processing, and middleware, expanding the crate's public API. It adds dependencies on Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant Service
Client->>Middleware: Send ServiceRequest
Middleware->>Service: Next::call(ServiceRequest)
Service-->>Middleware: ServiceResponse / Error
Middleware-->>Client: ServiceResponse / Error
sequenceDiagram
participant Handler
participant Extractor
participant Payload
Handler->>Extractor: from_message_request(&MessageRequest, &mut Payload)
Extractor->>Payload: advance / remaining
Extractor-->>Handler: Extracted value or Error
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Reviewer's GuideThis PR establishes the core trait foundations by adding extractor, frame-processing, and middleware modules, standardizes server worker defaults and examples, updates documentation and roadmap status, and introduces new dependencies to support async traits and byte handling. Sequence diagram for the Middleware execution flowsequenceDiagram
title Middleware Execution Flow
participant Client
participant Middleware
participant NextService as "Next Service / Handler"
Client->>Middleware: call(request)
Middleware->>Middleware: Pre-request processing
Middleware->>NextService: next.call(request)
NextService-->>Middleware: Result~ServiceResponse, Error~
Middleware->>Middleware: Post-request processing
Middleware-->>Client: Result~ServiceResponse, Error~
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/extractor.rs:15` </location>
<code_context>
+
+/// Raw payload buffer handed to extractors.
+#[derive(Default)]
+pub struct Payload<'a> {
+ /// Incoming bytes not yet processed.
+ pub data: &'a [u8],
</code_context>
<issue_to_address>
Add methods to advance through the payload
Consider implementing an `advance(count: usize)` method to allow updating the internal slice and tracking how much of the payload has been consumed.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
#[derive(Default)]
pub struct Payload<'a> {
/// Incoming bytes not yet processed.
pub data: &'a [u8],
}
=======
#[derive(Default)]
pub struct Payload<'a> {
/// Incoming bytes not yet processed.
pub data: &'a [u8],
}
impl<'a> Payload<'a> {
/// Advances the payload by `count` bytes, consuming that many bytes from the front.
pub fn advance(&mut self, count: usize) {
let len = self.data.len();
let n = count.min(len);
self.data = &self.data[n..];
}
/// Returns the number of bytes remaining in the payload.
pub fn remaining(&self) -> usize {
self.data.len()
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/middleware.rs:50` </location>
<code_context>
+
+/// Factory for wrapping services with middleware.
+#[async_trait]
+pub trait Transform<S>: Send + Sync
+where
+ S: Service,
</code_context>
<issue_to_address>
Rename associated `Service` type for clarity
Consider renaming the `Service` type alias within `Transform` to avoid confusion with the `Service` trait, such as `Output` or `Transformed`, for better readability.
Suggested implementation:
```rust
/// Wrapped service produced by the middleware.
type Output: Service;
```
```rust
/// Create a new middleware service wrapping `service`.
async fn transform(&self, service: S) -> Self::Output;
```
</issue_to_address>
### Comment 3
<location> `src/extractor.rs:35` </location>
<code_context>
+
+/// Shared application state accessible to handlers.
+#[derive(Clone)]
+pub struct SharedState<T>(Arc<T>);
+
+impl<T> SharedState<T> {
</code_context>
<issue_to_address>
Consider bounding `T` with `Send + Sync`
Enforcing `T: Send + Sync` on `SharedState` helps ensure safe usage in concurrent async contexts.
Suggested implementation:
```rust
use std::marker::PhantomData;
/// Shared application state accessible to handlers.
#[derive(Clone)]
pub struct SharedState<T: Send + Sync>(Arc<T>);
impl<T: Send + Sync> SharedState<T> {
```
```rust
impl<T: Send + Sync> std::ops::Deref for SharedState<T> {
type Target = T;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…cessor` Docstrings generation was requested by @leynos. * #8 (comment) The following files were modified: * `src/extractor.rs` * `src/middleware.rs` * `src/server.rs`
|
Note Generated docstrings for this pull request at #9 |
…cessor` (#9) Docstrings generation was requested by @leynos. * #8 (comment) The following files were modified: * `src/extractor.rs` * `src/middleware.rs` * `src/server.rs` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Test module is nested inside impl SharedState, causing invalid syntax (link)
General comments:
- The
#[must_use]attribute is currently floating before the tests module—move it to directly annotate theSharedState::newmethod so it applies correctly. - Your doc example for
SharedState::newusesassert_eq!which requiresDebug + PartialEqonSharedState; either derive those traits or adjust the example to avoid a compile error.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `#[must_use]` attribute is currently floating before the tests module—move it to directly annotate the `SharedState::new` method so it applies correctly.
- Your doc example for `SharedState::new` uses `assert_eq!` which requires `Debug + PartialEq` on `SharedState`; either derive those traits or adjust the example to avoid a compile error.
## Individual Comments
### Comment 1
<location> `src/extractor.rs:58` </location>
<code_context>
+ /// Construct a new [`SharedState`].
+ #[must_use]
+
+#[cfg(test)]
+mod tests {
+ use super::*;
</code_context>
<issue_to_address>
Test module is nested inside impl SharedState, causing invalid syntax
Move the `#[cfg(test)] mod tests` block outside the `impl SharedState` to resolve the syntax error and ensure correct scoping of methods.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
- In extractor.rs the #[cfg(test)] module is nested inside the SharedState impl—please move the tests outside the impl block and adjust indentation so it compiles correctly.
- The
MessageRequeststruct still has a TODO placeholder; either flesh out its fields or remove the comment to avoid confusion. - The server.rs docs repeat example blocks all marked
ignore; consolidate them and convert at least one into a real doc test for clearer documentation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In extractor.rs the #[cfg(test)] module is nested inside the SharedState impl—please move the tests outside the impl block and adjust indentation so it compiles correctly.
- The `MessageRequest` struct still has a TODO placeholder; either flesh out its fields or remove the comment to avoid confusion.
- The server.rs docs repeat example blocks all marked `ignore`; consolidate them and convert at least one into a real doc test for clearer documentation.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/server.rs (1)
31-48:⚠️ Potential issue
WireframeServer::newimplementation removed and docs malformed
- The method is only documented – the actual
pub fn newis gone, so everyWireframeServer::new(..)call will fail to compile.- The code block started with ```no_run is never closed, producing an “unclosed block” rustdoc error.
Apply the following patch:
@@ /// The server is initialised with a default worker count equal to the number of CPU cores. /// /// ```no_run @@ -/// .await -/// } -/// A new `WireframeServer` instance with the updated worker count. +/// .await +/// } +/// ``` + + /// Creates a new `WireframeServer` with sane defaults. + pub fn new(factory: F) -> Self { + Self { + factory, + listener: None, + workers: num_cpus::get().max(1), + } + }This restores functionality and fixes the doc markup.
♻️ Duplicate comments (2)
src/extractor.rs (1)
71-91:⚠️ Potential issueModule declared inside
implblock breaks compilation
#[cfg(test)] mod tests { .. }is placed inside theimpl SharedState<T>block.
Rust does not allow amoditem inside animpl, so the file will fail to compile.impl<T: Send + Sync> SharedState<T> { /* … existing methods … */ - #[cfg(test)] - mod tests { +} + +#[cfg(test)] +mod tests { use super::*; /* … */ }Move the whole test module outside the
implto restore valid syntax.src/middleware.rs (1)
82-84: Name clash betweenServicetrait and associated typeOutputThe associated type is still called
Output, which avoids the previousService/Serviceshadowing but is rather generic. Consider a more descriptive name such asWrappedorTransformedfor clarity.- /// Wrapped service produced by the middleware. - type Output: Service; + /// Service produced by the middleware. + type Wrapped: Service;This was suggested earlier and remains applicable.
🧹 Nitpick comments (4)
Cargo.toml (1)
12-13: Pinning to a hard-coded patch version can hinder semver compatibilityUnless you rely on a specific bug-fix,
"0.1"and"1"already resolve to the latest compatible patch. Locking further with=0.1.0or a fixed sub-minor can make downstream upgrades noisy. Consider using the caret syntax (the default) or adding acargo update/cargo denypolicy instead.docs/roadmap.md (1)
25-28: Spelling consistency: “Standardise” vs “Standardize”Project prose elsewhere (e.g. README) uses American spelling. Decide on one convention (en-GB or en-US) and stick to it to avoid grep-by-term surprises.
🧰 Tools
🪛 LanguageTool
[style] ~25-~25: Would you like to use the Oxford spelling “Standardize”? The spelling ‘Standardise’ is also correct.
Context: ...epting new connections. - [x] Standardise supporting trait definitions. P...(OXFORD_SPELLING_Z_NOT_S)
src/frame.rs (1)
17-22: Usebytes::BytesMutinstead ofVec<u8>for zero-copy efficiency
Vec<u8>incurs reallocations; the rest of the crate already depends onbytes. Returning/accepting&mut BytesMut(orBytes) gives you cheap slicing and reference-counted sharing, aligning with Tokio-style codecs.- async fn encode(&mut self, frame: &Self::Frame, dst: &mut Vec<u8>) -> Result<(), Self::Error>; + async fn encode( + &mut self, + frame: &Self::Frame, + dst: &mut bytes::BytesMut, + ) -> Result<(), Self::Error>;Applies analogously to
decode.src/middleware.rs (1)
45-60: Doctest snippet will not compile underrustdocHidden lines beginning with
#are still compiled.
your_crate::{ServiceRequest, ServiceResponse, Next, Service}is not a valid path in the current crate, thereforecargo test --docwill fail.Either:
-/// # use your_crate::{ServiceRequest, ServiceResponse, Next, Service}; +/// ```ignore +/// use my_crate::{ServiceRequest, ServiceResponse, Next, Service}; +/// ```or add the
ignoreflag to the whole example.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)docs/roadmap.md(1 hunks)src/extractor.rs(1 hunks)src/frame.rs(1 hunks)src/lib.rs(1 hunks)src/middleware.rs(1 hunks)src/server.rs(3 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/roadmap.md
[style] ~25-~25: Would you like to use the Oxford spelling “Standardize”? The spelling ‘Standardise’ is also correct.
Context: ...epting new connections. - [x] Standardise supporting trait definitions. P...
(OXFORD_SPELLING_Z_NOT_S)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: coverage
🔇 Additional comments (1)
src/lib.rs (1)
2-5: Ensure matching modules exist and are compiled
pub mod extractor;andpub mod middleware;must have correspondingmod.rsor*.rsfiles at compile time. CI will break otherwise. Double-check that they’re committed and feature-gated appropriately.
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
- In extractor.rs the
#[must_use]attribute appears to be dangling and thetestsmodule is nested inside theSharedStateimpl—move the tests out of the impl block and place the attribute directly above thenewmethod. - There are duplicate
# Examplesblocks in server.rs for methods likeworkers,bind, andrun—remove the redundant code snippets to keep the docs concise. - The async
FromMessageRequest::from_message_requestsignature uses a mutable borrow ofPayloadbut protocols generated byasync_traitrequire'staticfutures; consider adjusting lifetimes or switching to a synchronous extractor API to avoid potential borrow conflicts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In extractor.rs the `#[must_use]` attribute appears to be dangling and the `tests` module is nested inside the `SharedState` impl—move the tests out of the impl block and place the attribute directly above the `new` method.
- There are duplicate `# Examples` blocks in server.rs for methods like `workers`, `bind`, and `run`—remove the redundant code snippets to keep the docs concise.
- The async `FromMessageRequest::from_message_request` signature uses a mutable borrow of `Payload` but protocols generated by `async_trait` require `'static` futures; consider adjusting lifetimes or switching to a synchronous extractor API to avoid potential borrow conflicts.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Note Generated docstrings for this pull request at #10 |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
newconstructor initializations are missing (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/server.rs:40` </location>
<code_context>
/// ```
- /// let server = WireframeServer::new(|| WireframeApp::default());
- /// ```
- pub fn new(factory: F) -> Self {
- Self {
- factory,
- listener: None,
- workers: num_cpus::get(),
- }
- }
+ workers: num_cpus::get().max(1),
</code_context>
<issue_to_address>
`new` constructor initializations are missing
Please restore the `factory` and `listener` initializations in the `new` method to ensure `WireframeServer` is constructed correctly.
</issue_to_address>
### Comment 2
<location> `src/middleware.rs:30` </location>
<code_context>
+/// use wireframe::middleware::{ServiceRequest, ServiceResponse, Next, Service};
+/// ```
+ /// Service produced by the middleware.
+ type Wrapped: Service;
+ async fn transform(&self, service: S) -> Self::Wrapped;
+ /// let service = MyService::default();
+ /// let next = Next::new(&service);
</code_context>
<issue_to_address>
Invalid associated items inside `impl Next`
Move `type Wrapped` and `async fn transform` to a trait, such as `Transform`, or remove them from the `impl Next` block. Only `new` and `call` should be defined in `Next`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| pub fn new(factory: F) -> Self { | ||
| Self { | ||
| factory, | ||
| listener: None, | ||
| workers: num_cpus::get(), | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): new constructor initializations are missing
Please restore the factory and listener initializations in the new method to ensure WireframeServer is constructed correctly.
| type Wrapped: Service; | ||
| async fn transform(&self, service: S) -> Self::Wrapped; |
There was a problem hiding this comment.
issue: Invalid associated items inside impl Next
Move type Wrapped and async fn transform to a trait, such as Transform, or remove them from the impl Next block. Only new and call should be defined in Next.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- The
newconstructor is incomplete; missingSelf { ... }wrapper (link)
General comments:
- The #[cfg(test)] mod tests is declared inside the SharedState impl block—move it outside so the impl and tests compile correctly.
- The Next struct in middleware has duplicate/invalid transform definitions and no clear
newconstructor—consolidate into one correct API and ensure it compiles.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The #[cfg(test)] mod tests is declared inside the SharedState impl block—move it outside so the impl and tests compile correctly.
- The Next struct in middleware has duplicate/invalid transform definitions and no clear `new` constructor—consolidate into one correct API and ensure it compiles.
## Individual Comments
### Comment 1
<location> `src/server.rs:41` </location>
<code_context>
- workers: num_cpus::get(),
- }
- }
+ workers: num_cpus::get().max(1),
- /// Set the number of worker tasks to spawn.
</code_context>
<issue_to_address>
The `new` constructor is incomplete; missing `Self { ... }` wrapper
Please wrap the fields in `Self { factory, listener: None, workers: ... }` to correctly construct the struct.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
leynos
deleted the
codex/standardise-trait-definitions-for-frameprocessor
branch
|
@coderabbitai docstrings |
|
Tip For best results, initiate chat on the files or code changes. @leynos Sure! I’ll put together a plan to enrich the new public APIs with comprehensive Rust-style doc comments. I’ll prepare patches for • src/extractor.rs (MessageRequest, Payload, FromMessageRequest, SharedState) and make sure each item includes a concise summary, parameter / return sections, and examples where helpful. I’ll post the detailed change plan and code snippets in the next message. |
1 participant
Summary
FrameProcessor, state extractor, and middleware traitsTesting
cargo clippy -- -D warningscargo testnpx --yes markdownlint-cli2 '**/*.md'https://chatgpt.com/codex/tasks/task_e_684aab5541f48322bb628374734631cb
Summary by Sourcery
Implement foundational traits for message extraction, framing, and middleware; register new modules, update server defaults, dependencies, and documentation
New Features:
Enhancements:
Build:
Documentation:
Tests:
Summary by CodeRabbit