Conversation
Reviewer's GuideThis PR refactors PushQueues to use a new builder API: it introduces PushQueuesBuilder with fluent configuration methods, deprecates legacy constructors in favor of the builder, centralizes the queue setup logic into a single internal function, updates all code examples and documentation accordingly, and migrates the test suite to leverage the new builder. Class diagram for PushQueuesBuilder and PushQueues changesclassDiagram
class PushQueuesBuilder {
+high_capacity: usize
+low_capacity: usize
+rate: Option<usize>
+dlq: Option<mpsc::Sender<F>>
+new() PushQueuesBuilder
+high_capacity(capacity: usize) PushQueuesBuilder
+low_capacity(capacity: usize) PushQueuesBuilder
+rate(rate: Option<usize>) PushQueuesBuilder
+dlq(dlq: Option<mpsc::Sender<F>>) PushQueuesBuilder
+build() Result<(PushQueues<F>, PushHandle<F>), PushConfigError>
}
class PushQueues {
+high_priority_rx: mpsc::Receiver<F>
+low_priority_rx: mpsc::Receiver<F>
+builder() PushQueuesBuilder<F>
+close()
+build_with_rate_dlq(...)
+bounded(...)
+bounded_no_rate_limit(...)
+bounded_with_rate(...)
+bounded_with_rate_dlq(...)
}
PushQueuesBuilder --> PushQueues : builds
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 there - I've reviewed your changes - here's some feedback:
- Add a test for the builder’s error path (e.g. builder().rate(Some(0 or > MAX_PUSH_RATE)).build()) so we verify invalid rates are rejected.
- Enhance the PushQueuesBuilder doc comment with a minimal code example showing default usage (builder().build().unwrap()).
- Consider deriving Default for PushQueuesBuilder to give users a standard Default::default() entry point alongside builder().
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add a test for the builder’s error path (e.g. builder().rate(Some(0 or > MAX_PUSH_RATE)).build()) so we verify invalid rates are rejected.
- Enhance the PushQueuesBuilder doc comment with a minimal code example showing default usage (builder().build().unwrap()).
- Consider deriving Default for PushQueuesBuilder to give users a standard Default::default() entry point alongside builder().Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Note Reviews pausedUse the following commands to manage reviews:
Summary by CodeRabbit
WalkthroughIntroduce a new push module with a re-exported queues submodule. Add a builder-based API for PushQueues with options for capacities, optional rate limiting, and optional DLQ. Deprecate legacy constructors and route them through the builder. Update tests to construct queues via the builder and unwrap build results. Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test/Caller
participant B as PushQueuesBuilder
participant PQ as PushQueues
participant PH as PushHandle
participant RL as RateLimiter (optional)
participant DLQ as DLQ Sender (optional)
T->>PQ: PushQueues::builder()
activate B
T->>B: high_capacity(h)\nlow_capacity(l)\nrate(r?)\ndlq(dlq?)
T->>B: build()
alt r is Some
B->>RL: create with rate r
else r is None
Note over B,RL: No rate limiter
end
alt dlq is Some
B->>DLQ: attach DLQ
else
Note over B,DLQ: No DLQ
end
B->>PQ: construct queues (high/low)
B->>PH: create push handle
B-->>T: (PushQueues, PushHandle) or Error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/session_registry.rs (2)
9-12: Replace forbidden #[allow] with narrowly scoped #[expect] including reasonFollow lint policy: forbid
#[allow], prefer#[expect(..., reason = "...")].Apply:
-#[allow( - unused_braces, - reason = "rustc false positive for single line rstest fixtures" -)] +#[expect( + unused_braces, + reason = "rustc false positive for single-line rstest fixtures" +)]
16-19: Replace forbidden #[allow] with #[expect] on second fixtureKeep policy consistent.
Apply:
-#[allow( - unused_braces, - reason = "rustc false positive for single line rstest fixtures" -)] +#[expect( + unused_braces, + reason = "rustc false positive for single-line rstest fixtures" +)]tests/push.rs (2)
33-35: Fix docstring: variant is QueueFull, not Full.Keep docs accurate to the public API.
-/// return `PushError::Full` once the queue is at capacity. +/// return `PushError::QueueFull` once the queue is at capacity.
1-210: Remove all remainingunwrapcalls insrc/andtests/Found 46
unwrapoccurrences across 20 files. Address each as follows:• src/push/queues.rs (14 calls)
• src/extractor.rs (5 calls)
• tests/push.rs (4 calls)
• tests/connection_actor.rs (3 calls)
• src/server/config/binding.rs (3 calls)
• tests/world.rs (2 calls)
• tests/wireframe_protocol.rs (2 calls)
• tests/stream_end.rs (2 calls)
• tests/session_registry.rs (1 call)
• tests/server.rs (1 call)
• tests/push_policies.rs (1 call)
• tests/correlation_id.rs (1 call)
• tests/async_stream.rs (1 call)
• tests/advanced/interaction_fuzz.rs (1 call)
• tests/advanced/concurrency_loom.rs (1 call)
• src/server/connection.rs (1 call)
• src/frame/tests.rs (1 call)
• src/app.rs (1 call)Replace production‐code
unwrapuses with proper error propagation (e.g.?,map_err, orexpectwith descriptive message). In tests, useexpect("descriptive failure message")instead of rawunwrap.Review each file and refactor accordingly.
src/push/queues.rs (1)
53-91: Optionally derive errors via thiserror for cleaner impls.Reduce boilerplate and standardise error formatting.
+use thiserror::Error; @@ -#[derive(Debug)] -pub enum PushError { +#[derive(Debug, Error)] +pub enum PushError { - QueueFull, + #[error("push queue full")] + QueueFull, - Closed, + #[error("push queue closed")] + Closed, } @@ -#[derive(Debug)] -pub enum PushConfigError { +#[derive(Debug, Error)] +pub enum PushConfigError { - InvalidRate(usize), + #[error("invalid rate {0}; must be between 1 and {MAX_PUSH_RATE}")] + InvalidRate(usize), + #[error("invalid capacities; high={high}, low={low}; each must be ≥ 1")] + InvalidCapacity { high: usize, low: usize }, } - -impl std::fmt::Display for PushError { /* remove manual impl */ } -impl std::fmt::Display for PushConfigError { /* remove manual impl */ } - -impl std::error::Error for PushError {} -impl std::error::Error for PushConfigError {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
src/push/mod.rs(1 hunks)src/push/queues.rs(8 hunks)tests/advanced/concurrency_loom.rs(1 hunks)tests/advanced/interaction_fuzz.rs(1 hunks)tests/async_stream.rs(1 hunks)tests/connection_actor.rs(3 hunks)tests/correlation_id.rs(1 hunks)tests/push.rs(8 hunks)tests/push_policies.rs(3 hunks)tests/session_registry.rs(1 hunks)tests/stream_end.rs(2 hunks)tests/wireframe_protocol.rs(2 hunks)tests/world.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless 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
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
tests/session_registry.rssrc/push/mod.rstests/correlation_id.rstests/advanced/concurrency_loom.rstests/advanced/interaction_fuzz.rstests/async_stream.rstests/stream_end.rstests/world.rstests/connection_actor.rstests/wireframe_protocol.rstests/push_policies.rssrc/push/queues.rstests/push.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.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
tests/session_registry.rssrc/push/mod.rstests/correlation_id.rstests/advanced/concurrency_loom.rstests/advanced/interaction_fuzz.rstests/async_stream.rstests/stream_end.rstests/world.rstests/connection_actor.rstests/wireframe_protocol.rstests/push_policies.rssrc/push/queues.rstests/push.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/session_registry.rssrc/push/mod.rstests/correlation_id.rstests/advanced/concurrency_loom.rstests/advanced/interaction_fuzz.rstests/async_stream.rstests/stream_end.rstests/world.rstests/connection_actor.rstests/wireframe_protocol.rstests/push_policies.rssrc/push/queues.rstests/push.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/session_registry.rstests/correlation_id.rstests/advanced/concurrency_loom.rstests/advanced/interaction_fuzz.rstests/async_stream.rstests/stream_end.rstests/world.rstests/connection_actor.rstests/wireframe_protocol.rstests/push_policies.rstests/push.rs
🧬 Code graph analysis (12)
tests/session_registry.rs (1)
src/push/queues.rs (1)
builder(359-359)
src/push/mod.rs (1)
tests/connection_actor.rs (1)
queues(25-31)
tests/correlation_id.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/advanced/concurrency_loom.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/advanced/interaction_fuzz.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/async_stream.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/stream_end.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/world.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/connection_actor.rs (1)
src/push/queues.rs (1)
builder(359-359)
tests/wireframe_protocol.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/push_policies.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/push.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
🔇 Additional comments (5)
src/push/mod.rs (1)
3-5: Verify wildcard re-export lint and public module exposure inpush::queues
- File:
src/push/mod.rs(lines 3–5)- Verified no remaining usages of deprecated
PushQueues::bounded*constructors.- Decide whether to expose the
queuesmodule itself by changingmod queues;topub mod queues;.Apply this optional refactor:
-mod queues; +pub mod queues; -pub use queues::*; +// If Clippy flags wildcard imports, re-export specific items instead (preferred). +// e.g.: +// pub use queues::{PushQueues, PushQueuesBuilder, PushHandle}; +pub use queues::*;
- Verify whether Clippy currently warns on
pub use queues::*;under thewildcard_importslint in your stable toolchain.- Confirm that exposing
push::queuesaligns with your public API and doesn’t introduce unintended breakage.tests/push_policies.rs (2)
83-89: LGTM: Builder usage with explicit rate/DLQ and expect is consistent.
147-153: LGTM: Builder initialisation and explicit expect message read well.tests/push.rs (1)
81-87: LGTM: Builder with rate(Some/None) and explicit expect messages.Also applies to: 119-125, 140-146, 186-192
src/push/queues.rs (1)
293-355: LGTM: Builder API design and defaults mirror the legacy constructors cleanly.Chaining, defaults, and Result-returning build are idiomatic.
| @@ -0,0 +1,5 @@ | |||
| //! Prioritised queues used for asynchronously pushing frames to a connection. | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Elevate module docs with an example and deprecation note
Document the builder-centric API here with a short example and call out deprecated constructors to meet the public API documentation guideline.
Apply:
-//! Prioritised queues used for asynchronously pushing frames to a connection.
+//! Prioritised queues used for asynchronously pushing frames to a connection.
+//!
+//! Example
+//! -------
+//! Build queues and a handle using the builder:
+//!
+//! ```ignore
+//! use wireframe::push::PushQueues;
+//!
+//! let (queues, handle) = PushQueues::<u8>::builder()
+//! .high_capacity(8)
+//! .low_capacity(8)
+//! .build()
+//! .expect("failed to build push queues");
+//! ```
+//!
+//! Note: legacy `bounded*` constructors are deprecated; use `PushQueues::builder()` instead.🤖 Prompt for AI Agents
In src/push/mod.rs around lines 1 to 1, augment the module-level documentation
to include a short builder-centric example and a deprecation note: add a doc
example showing how to construct PushQueues using PushQueues::<T>::builder()
with high_capacity, low_capacity and build() (including expect), and append a
single-line note that legacy bounded* constructors are deprecated and users
should use PushQueues::builder() instead; ensure the example is fenced as a Rust
doc test and the deprecation note is concise and placed in the module docs.
| //! Queue management used by [`PushHandle`] and [`PushQueues`]. | ||
| //! | ||
| //! `PushQueues` maintain separate high- and low-priority channels so | ||
| //! background tasks can send messages without blocking the request/response | ||
| //! cycle. Producers interact with these queues through a cloneable | ||
| //! [`PushHandle`]. Queued frames are delivered in FIFO order within each | ||
| //! priority level. An optional rate limiter caps throughput at | ||
| //! [`MAX_PUSH_RATE`] pushes per second. | ||
| //! Provides the core implementation for prioritised queues delivering frames | ||
| //! to a connection. Background tasks can send messages without blocking the | ||
| //! request/response cycle. Frames maintain FIFO order within each priority | ||
| //! level. An optional rate limiter caps throughput at [`MAX_PUSH_RATE`] pushes | ||
| //! per second. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Split this module; file length exceeds the 400-line guideline.
Extract builder + errors into submodules (e.g., push/queues/{builder.rs, errors.rs, handle.rs}) and re-export from mod.rs.
| /// .high_capacity(1) | ||
| /// .low_capacity(1) | ||
| /// .rate(Some(1)) | ||
| /// .build() | ||
| /// .unwrap(); | ||
| /// handle.push_high_priority(42u8).await.unwrap(); | ||
| /// let (priority, frame) = queues.recv().await.unwrap(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use expect in doctest.
Avoid unwrap in examples per house style.
- /// .build()
- /// .unwrap();
+ /// .build()
+ /// .expect("failed to build queues");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// let (mut queues, handle) = PushQueues::builder() | |
| /// .high_capacity(1) | |
| /// .low_capacity(1) | |
| /// .rate(Some(1)) | |
| /// .build() | |
| /// .unwrap(); | |
| /// handle.push_high_priority(42u8).await.unwrap(); | |
| /// let (mut queues, handle) = PushQueues::builder() | |
| /// .high_capacity(1) | |
| /// .low_capacity(1) | |
| /// .rate(Some(1)) | |
| /// .build() | |
| /// .expect("failed to build queues"); | |
| /// handle.push_high_priority(42u8).await.unwrap(); |
🤖 Prompt for AI Agents
In src/push/queues.rs around lines 147 to 153, the doctest uses unwrap() which
violates house style; replace unwrap() calls with expect(...) that include
brief, clear failure messages (e.g., "failed to build PushQueues" and "failed to
push_high_priority") so the example still panics with useful context while
avoiding unwrap().
| /// .high_capacity(1) | ||
| /// .low_capacity(1) | ||
| /// .rate(Some(1)) | ||
| /// .build() | ||
| /// .unwrap(); | ||
| /// handle.push_low_priority(10u8).await.unwrap(); | ||
| /// let (priority, frame) = queues.recv().await.unwrap(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use expect in doctest.
- /// .build()
- /// .unwrap();
+ /// .build()
+ /// .expect("failed to build queues");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// let (mut queues, handle) = PushQueues::builder() | |
| /// .high_capacity(1) | |
| /// .low_capacity(1) | |
| /// .rate(Some(1)) | |
| /// .build() | |
| /// .unwrap(); | |
| /// handle.push_low_priority(10u8).await.unwrap(); | |
| /// let (mut queues, handle) = PushQueues::builder() | |
| /// .high_capacity(1) | |
| /// .low_capacity(1) | |
| /// .rate(Some(1)) | |
| /// .build() | |
| /// .expect("failed to build queues"); | |
| /// handle.push_low_priority(10u8).await.unwrap(); |
🤖 Prompt for AI Agents
In src/push/queues.rs around lines 178 to 184, the doctest uses unwrap() which
is discouraged; replace unwrap() calls with expect(...) that include clear
messages (e.g., .build().expect("failed to build PushQueues") and
.await.expect("push_low_priority failed")) so test failures give helpful
context; update both unwraps in the snippet accordingly.
| /// .high_capacity(1) | ||
| /// .low_capacity(1) | ||
| /// .rate(None) | ||
| /// .dlq(Some(dlq_tx)) | ||
| /// .build() | ||
| /// .unwrap(); | ||
| /// handle.push_high_priority(1u8).await.unwrap(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use expect in doctest.
- /// .build()
- /// .unwrap();
+ /// .build()
+ /// .expect("failed to build queues");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// let (mut queues, handle) = PushQueues::builder() | |
| /// .high_capacity(1) | |
| /// .low_capacity(1) | |
| /// .rate(None) | |
| /// .dlq(Some(dlq_tx)) | |
| /// .build() | |
| /// .unwrap(); | |
| /// let (mut queues, handle) = PushQueues::builder() | |
| /// .high_capacity(1) | |
| /// .low_capacity(1) | |
| /// .rate(None) | |
| /// .dlq(Some(dlq_tx)) | |
| /// .build() | |
| /// .expect("failed to build queues"); |
🤖 Prompt for AI Agents
In src/push/queues.rs around lines 231 to 237, the doctest currently ends with
.build().unwrap(); — replace the unwrap() with expect(...) to provide a clear
failure message in tests; update the line to use .build().expect("failed to
build PushQueues in doctest") so the doctest fails with a helpful message
instead of panicking without context.
| let (queues, handle) = PushQueues::builder() | ||
| .high_capacity(1) | ||
| .low_capacity(1) | ||
| .build() | ||
| .unwrap(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Repeat: replace unwrap() and make type explicit
Mirror the safer, consistent pattern here too.
- let (queues, handle) = PushQueues::builder()
+ let (queues, handle) = PushQueues::<u8>::builder()
.high_capacity(1)
.low_capacity(1)
.build()
- .unwrap();
+ .expect("failed to build push queues");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (queues, handle) = PushQueues::builder() | |
| .high_capacity(1) | |
| .low_capacity(1) | |
| .build() | |
| .unwrap(); | |
| let (queues, handle) = PushQueues::<u8>::builder() | |
| .high_capacity(1) | |
| .low_capacity(1) | |
| .build() | |
| .expect("failed to build push queues"); |
🤖 Prompt for AI Agents
In tests/stream_end.rs around lines 58 to 62, replace the .unwrap() call on
.build() with explicit error handling (either propagate the Result with ? from
the test function or call expect("...") with a clear message) and declare the
tuple's type explicitly (e.g. let (queues, handle): (PushQueues<...>, ... ) =
...) so the types are clear and unwrap() is removed.
| let (queues, handle) = PushQueues::builder() | ||
| .high_capacity(1) | ||
| .low_capacity(1) | ||
| .build() | ||
| .unwrap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace .unwrap() with .expect(...) for builder
Meet the .expect() preference and provide actionable error context.
Apply:
- let (queues, handle) = PushQueues::builder()
+ let (queues, handle) = PushQueues::builder()
.high_capacity(1)
.low_capacity(1)
- .build()
- .unwrap();
+ .build()
+ .expect("failed to build push queues for protocol hooks test")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (queues, handle) = PushQueues::builder() | |
| .high_capacity(1) | |
| .low_capacity(1) | |
| .build() | |
| .unwrap(); | |
| let (queues, handle) = PushQueues::builder() | |
| .high_capacity(1) | |
| .low_capacity(1) | |
| .build() | |
| .expect("failed to build push queues for protocol hooks test"); |
🤖 Prompt for AI Agents
In tests/wireframe_protocol.rs around lines 59 to 63, the PushQueues builder
call uses .unwrap(), replace it with .expect(...) to provide actionable error
context; change the call to .expect("failed to build PushQueues with
high_capacity=1 low_capacity=1") (or similar descriptive message) so test
failures show why the builder panicked.
| let (queues, handle) = PushQueues::builder() | ||
| .high_capacity(8) | ||
| .low_capacity(8) | ||
| .build() | ||
| .unwrap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace .unwrap() with .expect(...) for builder (second occurrence)
Keep tests consistent with lint policy.
Apply:
- let (queues, handle) = PushQueues::builder()
+ let (queues, handle) = PushQueues::builder()
.high_capacity(8)
.low_capacity(8)
- .build()
- .unwrap();
+ .build()
+ .expect("failed to build push queues for connection_actor protocol test")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (queues, handle) = PushQueues::builder() | |
| .high_capacity(8) | |
| .low_capacity(8) | |
| .build() | |
| .unwrap(); | |
| let (queues, handle) = PushQueues::builder() | |
| .high_capacity(8) | |
| .low_capacity(8) | |
| .build() | |
| .expect("failed to build push queues for connection_actor protocol test"); |
🤖 Prompt for AI Agents
In tests/wireframe_protocol.rs around lines 87 to 91, replace the second
.unwrap() call on
PushQueues::builder().high_capacity(8).low_capacity(8).build().unwrap() with
.expect(...) to follow the lint policy; change it to .expect("failed to build
PushQueues in test") (or a similar concise message) so the test panics with a
clear error message instead of using unwrap().
| let (queues, handle) = PushQueues::builder() | ||
| .high_capacity(1) | ||
| .low_capacity(1) | ||
| .build() | ||
| .unwrap(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Use expect() and explicit generic (Envelope) for the builder
Conform to lint policy and clarify the frame type at construction.
- let (queues, handle) = PushQueues::builder()
+ let (queues, handle) = PushQueues::<Envelope>::builder()
.high_capacity(1)
.low_capacity(1)
.build()
- .unwrap();
+ .expect("failed to build push queues");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (queues, handle) = PushQueues::builder() | |
| .high_capacity(1) | |
| .low_capacity(1) | |
| .build() | |
| .unwrap(); | |
| let (queues, handle) = PushQueues::<Envelope>::builder() | |
| .high_capacity(1) | |
| .low_capacity(1) | |
| .build() | |
| .expect("failed to build push queues"); |
🤖 Prompt for AI Agents
In tests/world.rs around lines 145 to 149, the PushQueues builder call uses
unwrap() and omits the explicit frame type; change PushQueues::builder() to
specify the Envelope generic (e.g. PushQueues::<Envelope>::builder()) and
replace .unwrap() with .expect("failed to build PushQueues") (or a similar
descriptive message) so the code conforms to lint policy and clarifies the frame
type at construction.
| let (queues, handle) = PushQueues::builder() | ||
| .high_capacity(1) | ||
| .low_capacity(1) | ||
| .build() | ||
| .unwrap(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Apply the same fix: expect() over unwrap() and explicit u8
Keep the tests consistent and lint-clean.
- let (queues, handle) = PushQueues::builder()
+ let (queues, handle) = PushQueues::<u8>::builder()
.high_capacity(1)
.low_capacity(1)
.build()
- .unwrap();
+ .expect("failed to build push queues");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (queues, handle) = PushQueues::builder() | |
| .high_capacity(1) | |
| .low_capacity(1) | |
| .build() | |
| .unwrap(); | |
| let (queues, handle) = PushQueues::<u8>::builder() | |
| .high_capacity(1) | |
| .low_capacity(1) | |
| .build() | |
| .expect("failed to build push queues"); |
🤖 Prompt for AI Agents
In tests/world.rs around lines 187 to 191, replace the use of .unwrap() on the
PushQueues::builder() result with .expect(...) and make the capacity literals
explicitly u8 (e.g. 1u8) so the test is lint-clean and consistent; update the
call to .build().expect("failed to build PushQueues") and change
.high_capacity(1) and .low_capacity(1) to .high_capacity(1u8) and
.low_capacity(1u8).
|
@coderabbitai please create a new GitHub issue for the following: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create a GitHub issue for removing the unwrap calls across the codebase. @leynos I've created a GitHub issue to track the removal of unwrap calls across the codebase. The issue includes the detailed breakdown of files and occurrence counts you provided, along with the specific guidance for handling production code versus test code differently. 🏁 Script executed: Length of output: 1571 Perfect! I've successfully created GitHub issue #337 for removing the unwrap calls across the codebase. The issue has been assigned to you and includes:
You can track the progress at: #337 |
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos Let me check the current state of the code to see if those items have been addressed. Based on my examination of the codebase, here's the status of the three items: ❌ Not Resolved:
All three items remain unaddressed in the current PR. Add tests for error conditions, improve documentation with examples, and consider adding 🏁 Script executed: Length of output: 2506 🏁 Script executed: Length of output: 3500 🏁 Script executed: Length of output: 996 @leynos Based on my examination of the current codebase, here's the status of the three items: ❌ All three items remain unresolved:
The validation logic exists (rates are checked against 🏁 Script executed: Length of output: 2591 @leynos Based on my thorough examination of the codebase, none of the three items have been resolved yet: ❌ Unresolved Items:
All three items still need to be addressed before this PR is complete. |
1 participant
Summary
PushQueuesBuilderwith configuration settersbounded*constructors and exposePushQueues::builderTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_68aeba8d630c8322bcab55de80b2ef06
Summary by Sourcery
Introduce a builder pattern for PushQueues to allow flexible configuration of capacities, rate limiting, and dead-letter queues, deprecate the old constructors in favor of PushQueues::builder, and update tests and docs accordingly.
New Features:
Enhancements:
Tests: