Extract config tests into dedicated module#278
Conversation
Reviewer's GuideThis PR refactors the server configuration API by extracting its tests into a dedicated module behind cfg(test), restructures the config builder into its own module (removing type-level bound states), and enhances the WireframeServer runtime methods with improved documentation and example usage. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughRemove the original Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WireframeServer
participant OS
participant Tokio
User->>WireframeServer: new(factory)
User->>WireframeServer: workers(count)
User->>WireframeServer: with_preamble(P)
User->>WireframeServer: on_preamble_decode_success(handler)
User->>WireframeServer: on_preamble_decode_failure(handler)
User->>WireframeServer: ready_signal(tx)
User->>WireframeServer: bind(addr)
WireframeServer->>OS: create StdTcpListener
WireframeServer->>Tokio: convert to Tokio TcpListener
WireframeServer->>WireframeServer: update listener field
User->>WireframeServer: local_addr()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit Inference Engine (AGENTS.md)
Files:
⚙️ CodeRabbit Configuration File
Files:
🧬 Code Graph Analysis (1)src/server/config/mod.rs (3)
🔇 Additional comments (5)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| tests.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
![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 found some issues that need to be addressed.
Blocking issues:
- Use of
#[allow(clippy::too_many_arguments)]is forbidden; use a narrowly scoped#[expect(...)]with a reason instead. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/server/config/mod.rs:20` </location>
<code_context>
-use super::{Bound, PreambleCallback, PreambleErrorCallback, Unbound, WireframeServer};
-use crate::{app::WireframeApp, preamble::Preamble};
-
-impl<F> WireframeServer<F, (), Unbound>
-where
- F: Fn() -> WireframeApp + Send + Sync + Clone + 'static,
</code_context>
<issue_to_address>
Consider replacing the Unbound/Bound state generics with an Option<Arc<TcpListener>> field to unify the server implementation.
Here’s a sketch of how you can collapse the two‐state generic (“Unbound” vs “Bound”) into one struct with an `Option<Arc<TcpListener>>`. That lets you:
* remove both the `impl<…Unbound>` and `impl<…Bound>` blocks
* have one unified `bind` / `bind_listener` / `local_addr` implementation
* keep *exactly* the same public API and behavior
```rust
pub struct WireframeServer<F, P> {
factory: F,
workers: usize,
on_preamble_success: Option<PreambleCallback<P>>,
on_preamble_failure: Option<PreambleErrorCallback>,
ready_tx: Option<oneshot::Sender<()>>,
listener: Option<Arc<TcpListener>>,
_preamble: PhantomData<P>,
}
impl<F, P> WireframeServer<F, P>
where
F: Fn() -> WireframeApp + Send + Sync + Clone + 'static,
P: Preamble,
{
/// Returns the bound address, or `None` if not yet bound.
#[must_use]
pub fn local_addr(&self) -> Option<SocketAddr> {
self.listener
.as_ref()
.and_then(|l| l.local_addr().ok())
}
/// Bind to a fresh address.
pub fn bind(mut self, addr: SocketAddr) -> io::Result<Self> {
let std = StdTcpListener::bind(addr)?;
self.bind_listener(std)
}
/// Bind to an existing `StdTcpListener`.
pub fn bind_listener(mut self, mut std: StdTcpListener) -> io::Result<Self> {
std.set_nonblocking(true)?;
let tokio = TcpListener::from_std(std)?;
self.listener = Some(Arc::new(tokio));
Ok(self)
}
// ... keep your `new`, `with_preamble`, `workers`, callbacks, ready_signal, etc.
}
```
Steps to apply:
1. Add `listener: Option<Arc<TcpListener>>` to the struct and drop the `state: Unbound/Bound` generic.
2. Collapse both `impl<…, Unbound>` and `impl<…, Bound>` blocks into the single `impl<F, P> …` above.
3. Move your existing `bind_std_listener` logic into the unified `bind_listener(...)` method.
This preserves *all* your callbacks, worker‐count logic, and type‐parameters, but removes the two‐state boilerplate completely.
</issue_to_address>
### Comment 2
<location> `src/server/config/mod.rs:199` </location>
<code_context>
+ }
+}
+
+#[allow(clippy::too_many_arguments)]
+fn bind_std_listener<F, T>(
+ factory: F,
</code_context>
<issue_to_address>
Use of `#[allow(clippy::too_many_arguments)]` is forbidden; use a narrowly scoped `#[expect(...)]` with a reason instead.
The review instructions explicitly forbid the use of `#[allow]` for lints. Please replace this with a narrowly scoped `#[expect(clippy::too_many_arguments, reason = "...")]` and provide a justification. Blanket or group suppression is not permitted.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/server/config.rs(0 hunks)src/server/config/mod.rs(1 hunks)src/server/config/tests.rs(1 hunks)
💤 Files with no reviewable changes (1)
- src/server/config.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status....
Files:
src/server/config/tests.rssrc/server/config/mod.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
src/server/config/tests.rssrc/server/config/mod.rs
🔇 Additional comments (1)
src/server/config/tests.rs (1)
41-49: Well-structured edge case testing!Good coverage of worker configuration including the zero-worker edge case that should default to 1.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
src/server/config/tests.rs (3)
1-1: Expand the module documentation to explain test coverage scope.The module documentation is too brief. Add details about what aspects of server configuration are being tested and the testing strategy employed.
-//! Tests for server configuration utilities. +//! Tests for server configuration utilities. +//! +//! This module provides comprehensive test coverage for the `WireframeServer` configuration, +//! including worker count settings, binding behaviour, preamble handling, callback registration, +//! and method chaining. Tests use `rstest` fixtures for shared setup and parameterised cases.
22-25: Remove duplication of the default worker count logic.This duplicates the logic from
WireframeServer::new. If the implementation changes, tests could pass incorrectly due to parallel changes in both places. Test against the actual behaviour instead.
30-30: Split the compound assertion for clearer test failures.Compound assertions make debugging test failures harder. Split into separate assertions.
- assert!(server.worker_count() >= 1 && server.local_addr().is_none()); + assert!(server.worker_count() >= 1); + assert!(server.local_addr().is_none());src/server/config/mod.rs (7)
29-33: Add usage example to function documentation.Function documentation must include clear examples demonstrating usage and outcome.
/// Create a new `WireframeServer` from the given application factory. /// /// The worker count defaults to the number of available CPU cores (or 1 if this cannot be /// determined). The TCP listener is unset; call [`bind`](Self::bind) before running the /// server. + /// + /// # Example + /// + /// ``` + /// use wireframe::server::WireframeServer; + /// + /// let server = WireframeServer::new(|| MyApp::new()); + /// assert!(server.worker_count() >= 1); + /// ```
53-55: Add usage example to function documentation./// Converts the server to use a custom preamble type for incoming connections. /// /// Calling this method drops any previously configured preamble decode callbacks. + /// + /// # Example + /// + /// ``` + /// use wireframe::server::WireframeServer; + /// + /// let server = WireframeServer::new(|| MyApp::new()) + /// .with_preamble::<MyPreamble>(); + /// ```
78-78: Add usage example to function documentation./// Set the number of worker tasks to spawn for the server. + /// + /// # Example + /// + /// ``` + /// let server = WireframeServer::new(|| MyApp::new()) + /// .workers(4); + /// assert_eq!(server.worker_count(), 4); + /// ```
85-85: Add usage example to function documentation./// Register a callback invoked when the connection preamble decodes successfully. + /// + /// # Example + /// + /// ``` + /// let server = server.on_preamble_decode_success(|preamble, stream| { + /// Box::pin(async move { + /// println!("Preamble decoded: {:?}", preamble); + /// Ok(()) + /// }) + /// }); + /// ```
98-98: Add usage example to function documentation./// Register a callback invoked when the connection preamble fails to decode. + /// + /// # Example + /// + /// ``` + /// let server = server.on_preamble_decode_failure(|error| { + /// eprintln!("Preamble decode failed: {}", error); + /// }); + /// ```
108-108: Add usage example to function documentation./// Configure a channel used to signal when the server is ready to accept connections. + /// + /// # Example + /// + /// ``` + /// let (tx, rx) = tokio::sync::oneshot::channel(); + /// let server = server.ready_signal(tx); + /// // Server will send on tx when ready + /// ```
156-159: Add usage example to function documentation./// Bind the server to the given address and create a listener. /// /// # Errors /// Returns an `io::Error` if binding or configuring the listener fails. + /// + /// # Example + /// + /// ``` + /// use std::net::SocketAddr; + /// + /// let addr: SocketAddr = "127.0.0.1:8080".parse().unwrap(); + /// let server = server.bind(addr)?; + /// ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/server/config/mod.rs(1 hunks)src/server/config/tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status....
Files:
src/server/config/tests.rssrc/server/config/mod.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
src/server/config/tests.rssrc/server/config/mod.rs
🧬 Code Graph Analysis (1)
src/server/config/tests.rs (2)
src/server/test_util.rs (3)
bind_server(31-38)free_port(23-29)server_with_preamble(40-45)src/server/config/mod.rs (7)
new(35-46)worker_count(118-118)local_addr(154-154)local_addr(184-184)workers(80-83)bind(160-163)bind(190-193)
🔇 Additional comments (4)
src/server/config/tests.rs (2)
41-49: Well-structured test with good edge case coverage.The test properly validates worker configuration including the edge case of zero workers being clamped to 1.
176-182: Excellent edge case testing for worker count boundaries.The test properly validates behaviour at the extreme boundaries of the worker count range.
src/server/config/mod.rs (2)
1-6: Good module documentation with clear explanation of design patterns.The documentation properly explains the module's purpose, builder pattern usage, and compile-time state tracking with phantom types.
124-144: Well-structured delegation to binding helper.Good separation of concerns with the helper method extracting fields and delegating to the standalone binding function.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/server/config/mod.rs (5)
97-102: Add usage example to function documentation.Function documentation must include clear examples demonstrating usage and outcome as per the coding guidelines.
/// Set the number of worker tasks to spawn for the server. + /// + /// # Examples + /// + /// ``` + /// use wireframe::{app::WireframeApp, server::WireframeServer}; + /// + /// let server = WireframeServer::new(|| WireframeApp::default()) + /// .workers(4); + /// assert_eq!(server.worker_count(), 4); + /// ``` #[must_use]
104-115: Add usage example to function documentation.Function documentation must include clear examples as per the coding guidelines.
/// Register a callback invoked when the connection preamble decodes successfully. + /// + /// # Examples + /// + /// ``` + /// use wireframe::{app::WireframeApp, server::WireframeServer}; + /// + /// let server = WireframeServer::new(|| WireframeApp::default()) + /// .on_preamble_decode_success(|preamble, stream| { + /// Box::pin(async move { + /// println!("Preamble decoded: {:?}", preamble); + /// Ok(()) + /// }) + /// }); + /// ``` #[must_use]
117-125: Add usage example to function documentation.Function documentation must include clear examples as per the coding guidelines.
/// Register a callback invoked when the connection preamble fails to decode. + /// + /// # Examples + /// + /// ``` + /// use wireframe::{app::WireframeApp, server::WireframeServer}; + /// + /// let server = WireframeServer::new(|| WireframeApp::default()) + /// .on_preamble_decode_failure(|error| { + /// eprintln!("Preamble decode failed: {}", error); + /// }); + /// ``` #[must_use]
127-132: Add usage example to function documentation.Function documentation must include clear examples as per the coding guidelines.
/// Configure a channel used to signal when the server is ready to accept connections. + /// + /// # Examples + /// + /// ``` + /// use tokio::sync::oneshot; + /// use wireframe::{app::WireframeApp, server::WireframeServer}; + /// + /// let (tx, rx) = oneshot::channel(); + /// let server = WireframeServer::new(|| WireframeApp::default()) + /// .ready_signal(tx); + /// // Server will send on tx when ready + /// ``` #[must_use]
145-152: Add usage example to function documentation.Function documentation must include clear examples as per the coding guidelines.
/// Bind to a fresh address. /// /// # Errors /// Returns an `io::Error` if binding or configuring the listener fails. + /// + /// # Examples + /// + /// ``` + /// use std::net::SocketAddr; + /// use wireframe::{app::WireframeApp, server::WireframeServer}; + /// + /// # fn main() -> std::io::Result<()> { + /// let addr: SocketAddr = "127.0.0.1:8080".parse().unwrap(); + /// let server = WireframeServer::new(|| WireframeApp::default()) + /// .bind(addr)?; + /// # Ok(()) + /// # } + /// ``` pub fn bind(self, addr: SocketAddr) -> io::Result<Self> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/server/config/mod.rs(1 hunks)src/server/mod.rs(2 hunks)src/server/runtime.rs(2 hunks)src/server/test_util.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status....
Files:
src/server/test_util.rssrc/server/mod.rssrc/server/runtime.rssrc/server/config/mod.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
src/server/test_util.rssrc/server/mod.rssrc/server/runtime.rssrc/server/config/mod.rs
🧬 Code Graph Analysis (1)
src/server/config/mod.rs (3)
tests/preamble.rs (1)
oneshot(71-71)src/server/test_util.rs (2)
WireframeServer(44-44)factory(18-20)src/server/config/tests.rs (2)
WireframeServer(57-57)WireframeServer(158-160)
🔇 Additional comments (6)
src/server/test_util.rs (1)
8-8: LGTM! Correctly reflects the simplifiedWireframeServertype.The removal of the
Boundimport and simplification of the return type aligns perfectly with the broader refactoring that eliminates the state type parameter.Also applies to: 31-31
src/server/mod.rs (1)
36-36: Excellent simplification of the server architecture.The removal of the state type parameter and replacement with
Option<Arc<TcpListener>>eliminates unnecessary complexity whilst maintaining the same functionality. This addresses the previous review suggestion perfectly.Also applies to: 60-60
src/server/runtime.rs (1)
25-25: Runtime correctly adapted to the simplified server structure.The removal of the
Boundwrapper and addition of explicit error handling for unbound listeners maintains the same behaviour whilst working with the simplifiedWireframeServerstruct.Also applies to: 59-63
src/server/config/mod.rs (3)
1-6: Module documentation appropriately describes the simplified design.The documentation correctly explains the fluent builder pattern and optional listener approach, which is clearer than the previous state machine design.
29-55: Well-documented constructor with sensible defaults.The
newfunction correctly initialises all fields and provides a clear usage example as required by the coding guidelines.
63-95: Correct preamble type conversion with proper callback reset.The function appropriately resets preamble callbacks when changing types and includes the required usage example.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/server/config/mod.rs (4)
139-144: Add missing documentation example.Function documentation must include clear examples as per the coding guidelines.
/// Configure a channel used to signal when the server is ready to accept connections. + /// + /// # Examples + /// + /// ``` + /// use tokio::sync::oneshot; + /// use wireframe::{app::WireframeApp, server::WireframeServer}; + /// + /// let (tx, rx) = oneshot::channel(); + /// let server = WireframeServer::new(|| WireframeApp::default()) + /// .ready_signal(tx); + /// // Server will send on tx when ready + /// ```
166-175: Add missing documentation example.Function documentation must include clear examples as per the coding guidelines.
/// Bind to an existing `StdTcpListener`. /// /// # Errors /// Returns an `io::Error` if configuring the listener fails. + /// + /// # Examples + /// + /// ``` + /// use std::net::TcpListener; + /// use wireframe::{app::WireframeApp, server::WireframeServer}; + /// + /// # fn main() -> std::io::Result<()> { + /// let std_listener = TcpListener::bind("127.0.0.1:8080")?; + /// let server = WireframeServer::new(|| WireframeApp::default()) + /// .bind_listener(std_listener)?; + /// # Ok(()) + /// # } + /// ```
97-106: Fix malformed documentation and add missing example.The documentation comment is incorrectly formatted and lacks the required usage example.
-/// Set the number of worker tasks to spawn for the server. - /// - /// # Example - /// - /// + /// Set the number of worker tasks to spawn for the server. + /// + /// # Examples + /// + /// ``` + /// use wireframe::{app::WireframeApp, server::WireframeServer}; + /// + /// let server = WireframeServer::new(|| WireframeApp::default()) + /// .workers(4); + /// assert_eq!(server.worker_count(), 4); + /// ```
125-137: Fix malformed documentation and add missing example.The documentation comment is incorrectly formatted and lacks the required usage example.
-/// Register a callback invoked when the connection preamble fails to decode. - /// - /// # Example - /// - /// + /// Register a callback invoked when the connection preamble fails to decode. + /// + /// # Examples + /// + /// ``` + /// use wireframe::{app::WireframeApp, server::WireframeServer}; + /// + /// let server = server.on_preamble_decode_failure(|error| { + /// eprintln!("Preamble decode failed: {}", error); + /// }); + /// ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/server/config/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status....
Files:
src/server/config/mod.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
src/server/config/mod.rs
🔇 Additional comments (5)
src/server/config/mod.rs (5)
1-7: LGTM! Module documentation meets requirements.The module documentation properly explains the purpose, builder pattern, and key functionality with appropriate detail as required by the coding guidelines.
8-23: LGTM! Clean imports and test module declaration.The imports are well-organised and the conditional test module inclusion follows proper conventions.
29-55: LGTM! Constructor implementation with proper defaults.The
newmethod correctly initialises the server with sensible defaults, usingavailable_parallelism()with a fallback to 1. The documentation example demonstrates proper usage.
63-95: LGTM! Type conversion method properly resets callbacks.The
with_preamblemethod correctly creates a new instance with the target preamble type whilst resetting preamble-related callbacks to prevent type mismatches. The documentation example is comprehensive.
146-155: LGTM! Simple accessor methods are well-implemented.Both
worker_countandlocal_addrmethods are correctly implemented with appropriate attributes and error handling.
leynos
force-pushed
the
codex/extract-test-module-from-config.rs
branch
from
f793c95 to
357e777
Compare
|
Caution An unexpected error occurred while opening a pull request: Reference does not exist - https://docs.github.com/rest/git/refs#update-a-reference |
Documents all public builder methods and runtime entrypoints with runnable doctests covering typical usage patterns. Enables consumers to see practical configuration examples without needing external samples.
Extracts repeated default worker count calculation into a dedicated helper Simplifies callback registration tests by removing unnecessary network setup Streamlines assertion logic and removes a redundant test for callback resets Updates variable naming for consistency and drops unused imports
leynos
force-pushed
the
codex/extract-test-module-from-config.rs
branch
from
357e777 to
7ffae35
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- The removal of the Bound/Unbound typestate makes binding errors arise at runtime; consider restoring typestate to enforce binding at compile time.
- The error returned when running an unbound server ('listener not bound') is generic; consider using a more descriptive error or a custom error type.
- src/server/config/mod.rs is quite large now; splitting it into smaller submodules (e.g., binding, preamble) could improve readability and maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The removal of the Bound/Unbound typestate makes binding errors arise at runtime; consider restoring typestate to enforce binding at compile time.
- The error returned when running an unbound server ('listener not bound') is generic; consider using a more descriptive error or a custom error type.
- src/server/config/mod.rs is quite large now; splitting it into smaller submodules (e.g., binding, preamble) could improve readability and maintainability.
## Individual Comments
### Comment 1
<location> `src/server/config/mod.rs:97` </location>
<code_context>
- F: Fn() -> WireframeApp + Send + Sync + Clone + 'static,
- T: Preamble,
-{
- /// Set the number of worker tasks to spawn for the server.
- #[must_use]
- pub fn workers(mut self, count: usize) -> Self {
</code_context>
<issue_to_address>
Consider replacing the repetitive setter and callback methods with builder macros to reduce boilerplate.
You can collapse all those repetitive setters into either a derived‐builder (via the derive_builder crate) or a tiny macro. Here’s an example of the macro approach which preserves your signatures but cuts ~200 LOC of boilerplate:
```rust
// at top of file
macro_rules! builder_setter {
($(#[$meta:meta])* $fn:ident, $field:ident, $ty:ty) => {
$(#[$meta])*
#[must_use]
pub fn $fn(mut self, val: $ty) -> Self {
self.$field = val;
self
}
};
}
macro_rules! builder_callback {
($fn:ident, $field:ident, $bound:ty) => {
#[must_use]
pub fn $fn<H>(mut self, handler: H) -> Self
where H: $bound {
self.$field = Some(Arc::new(handler));
self
}
};
}
```
Then inside your `impl<F,T> WireframeServer<F,T> { … }` you replace the manual methods with calls to those macros:
```rust
impl<F, T> WireframeServer<F, T>
where
F: Fn() -> WireframeApp + Send + Sync + Clone + 'static,
T: Preamble,
{
builder_setter!(
/// Set number of worker tasks (min 1).
workers, workers, usize
);
builder_setter!(
/// Configure a channel to signal readiness.
ready_signal, ready_tx, oneshot::Sender<()>
);
builder_callback!(
/// Callback for preamble decode success.
on_preamble_decode_success,
on_preamble_success,
for<'a> Fn(&'a T, &'a mut tokio::net::TcpStream) -> BoxFuture<'a, io::Result<()>> + Send + Sync + 'static
);
builder_callback!(
/// Callback for preamble decode failure.
on_preamble_decode_failure,
on_preamble_failure,
Fn(&DecodeError) + Send + Sync + 'static
);
// keep new(), with_preamble(), worker_count(), bind()/bind_listener(), etc.
}
```
This preserves all functionality, keeps your docs, and collapses the repeated `mut self`/`self` plumbing into two small macros.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1 participant
Summary
src/server/config/tests.rsto keep implementation concise#[cfg(test)]moduleTesting
make fmtmake lintmake testcloses #274
https://chatgpt.com/codex/tasks/task_e_68927e18d46c832285935b48d6d32742
Summary by Sourcery
Extract server configuration builder API into a dedicated config module, simplify WireframeServer state handling, and relocate configuration tests.
Enhancements:
Documentation:
Tests: