Fix compile errors and clippy #11
Conversation
Reviewer's GuideThis PR refactors server, middleware, and extractor modules to resolve compile and Clippy issues by introducing dedicated constructors, inlining and must-use annotations, trait implementations, relocating tests, and updating documentation with a runnable example. Class Diagram:
|
| Change | Details | Files |
|---|---|---|
| Introduce WireframeServer::new constructor and method annotations |
|
src/server.rs |
| Streamline Next API in middleware with inline and must-use |
|
src/middleware.rs |
| Extend SharedState with constructors and From implementations |
|
src/extractor.rs |
| Update roadmap documentation with runnable example |
|
docs/roadmap.md |
Tips and commands
Interacting with Sourcery
- Trigger a new review: Comment
@sourcery-ai reviewon the pull request. - Continue discussions: Reply directly to Sourcery's review comments.
- Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with@sourcery-ai issueto create an issue from it. - Generate a pull request title: Write
@sourcery-aianywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai titleon the pull request to (re-)generate the title at any time. - Generate a pull request summary: Write
@sourcery-ai summaryanywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment@sourcery-ai summaryon the pull request to
(re-)generate the summary at any time. - Generate reviewer's guide: Comment
@sourcery-ai guideon the pull
request to (re-)generate the reviewer's guide at any time. - Resolve all Sourcery comments: Comment
@sourcery-ai resolveon the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore. - Dismiss all Sourcery reviews: Comment
@sourcery-ai dismisson the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai reviewto trigger a new review!
Customizing Your Experience
Access your dashboard to:
- Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others. - Change the review language.
- Add, remove or edit custom review instructions.
- Adjust other review settings.
Getting Help
- Contact our support team for questions or feedback.
- Visit our documentation for detailed guides and information.
- Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 53 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 (4)
WalkthroughThis update marks the "Provide a minimal, runnable example" checklist item as complete in the roadmap. It introduces a full Rust example for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WireframeApp
participant WireframeServer
User->>WireframeApp: Define routes and handlers
User->>WireframeServer: Create server with app factory
WireframeServer->>WireframeServer: Set worker count
User->>WireframeServer: Start server (run)
WireframeServer->>WireframeApp: Handle incoming requests
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 (
|
![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 - here's some feedback:
- Double-check that removing the
transformmethod fromNextdoesn’t break your middleware trait implementations—re-expose or adapt it if it’s still required byTransform. - Verify that the new code snippet in
docs/roadmap.mdactually compiles against your public API (especially theServicealias and handler closure signature).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Double-check that removing the `transform` method from `Next` doesn’t break your middleware trait implementations—re-expose or adapt it if it’s still required by `Transform`.
- Verify that the new code snippet in `docs/roadmap.md` actually compiles against your public API (especially the `Service` alias and handler closure signature).
## Individual Comments
### Comment 1
<location> `src/middleware.rs:34` </location>
<code_context>
- /// assert!(res.is_ok());
- /// ```
+ /// Propagates any error produced by the wrapped service.
pub async fn call(&self, req: ServiceRequest) -> Result<ServiceResponse, S::Error> {
self.service.call(req).await
}
</code_context>
<issue_to_address>
Add `#[must_use]` to async call to catch ignored futures
This ensures that dropped or unawaited futures from `call` are flagged, preventing silent errors in middleware chains.
</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: 0
🧹 Nitpick comments (7)
src/extractor.rs (2)
72-74: Consider providing ergonomic conversions in addition tonew.
SharedState::newis fine, but every caller must already have anArc<T>.
ProvidingFrom<Arc<T>>(and possiblyFrom<T>that internally wraps withArc::new) would make the type feel more “transparent” and avoid the extra clone-and-wrap ceremony.+impl<T: Send + Sync> From<Arc<T>> for SharedState<T> { + fn from(inner: Arc<T>) -> Self { + Self(inner) + } +} + +impl<T: Send + Sync> From<T> for SharedState<T> { + fn from(inner: T) -> Self { + Self(Arc::new(inner)) + } +}
108-114: Doc test could compile ‑ considerno_runinstead ofignore.The snippet now imports
wireframe::extractor::SharedState, which should compile.
Switching fromignoretono_runwill still skip execution but at least type-checks the example in CI.-/// ```ignore +/// ```no_runsrc/middleware.rs (2)
23-27:Next::newlooks good – suggest#[inline]for zero-cost wrapper.The constructor is purely a field initialiser; adding
#[inline]would let the optimiser remove the call overhead completely without affecting readability.- #[must_use] + #[inline] + #[must_use]
29-35: Clarify#[must_use]message oncall.The message says “call the returned future”, but the user actually needs to await it. A tiny wording tweak avoids confusion.
- #[must_use = "call the returned future"] + #[must_use = "await the returned future"]src/server.rs (2)
31-39: Minor redundancy in worker count initialisation.
num_cpus::get()never returns0, so.max(1)is unnecessary.- workers: num_cpus::get().max(1), + workers: num_cpus::get(),
41-45: Builder ergonomics: allow mutating an existing value.
workers(self, count)consumesself, preventing users from conditionally setting it later in fluent chains. A&mut selfreceiver keeps the chaining style while offering flexibility:-pub fn workers(mut self, count: usize) -> Self { - self.workers = count.max(1); - self -} +pub fn workers(&mut self, count: usize) -> &mut Self { + self.workers = count.max(1); + self +}Call-sites can still chain with
..workers(4)..thanks to the borrow checker.docs/roadmap.md (1)
33-57: Tidy up the example imports.
std::{future::Future, pin::Pin}are unused after the recent edits. Removing them keeps the example minimal and avoids dead-code warnings.-use std::{future::Future, pin::Pin}; +// No extra imports required
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/roadmap.md(1 hunks)src/extractor.rs(2 hunks)src/middleware.rs(2 hunks)src/server.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: coverage
|
@sourcery-ai review |
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/server.rs:43` </location>
<code_context>
- pub fn workers(mut self, count: usize) -> Self {
+ /// Set the number of worker tasks to spawn.
+ #[must_use]
+ pub fn workers(&mut self, count: usize) -> &mut Self {
self.workers = count.max(1);
self
</code_context>
<issue_to_address>
Builder method signature may break chaining on temporaries
With the new `&mut self` signature, you lose the ability to chain methods on temporaries. To keep fluent chaining, consider keeping the signature as `workers(self, count: usize) -> Self`.
</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 - here's some feedback:
- In WireframeServer::new you drop the
.max(1)on the default worker count—consider adding.max(1)there as well to guarantee at least one worker, matching the behavior of theworkerssetter. - The minimal example in docs/roadmap.md imports
Serviceas if it’s a concrete type—this may confuse users; either show a ready‐to‐use service implementation in the snippet or clarify the trait alias so the example compiles out of the box. - The diff in middleware.rs still shows duplicate
type Wrappedandasync fn transformdeclarations under theNextimpl—clean up those leftovers so only the intended constructor and call methods remain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In WireframeServer::new you drop the `.max(1)` on the default worker count—consider adding `.max(1)` there as well to guarantee at least one worker, matching the behavior of the `workers` setter.
- The minimal example in docs/roadmap.md imports `Service` as if it’s a concrete type—this may confuse users; either show a ready‐to‐use service implementation in the snippet or clarify the trait alias so the example compiles out of the box.
- The diff in middleware.rs still shows duplicate `type Wrapped` and `async fn transform` declarations under the `Next` impl—clean up those leftovers so only the intended constructor and call methods remain.
## Individual Comments
### Comment 1
<location> `src/server.rs:31` </location>
<code_context>
- /// let server = WireframeServer::new(factory);
- /// ```
- workers: num_cpus::get().max(1),
+ /// Construct a new server using the supplied application factory.
+ #[must_use]
+ pub fn new(factory: F) -> Self {
</code_context>
<issue_to_address>
Include default worker count behavior in documentation
Please note in the documentation that the default worker count is determined by the number of CPU cores.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
/// Construct a new server using the supplied application factory.
=======
/// Construct a new server using the supplied application factory.
///
/// By default, the number of worker threads is set to the number of CPU cores available.
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| /// let server = WireframeServer::new(factory); | ||
| /// ``` | ||
| workers: num_cpus::get().max(1), | ||
| /// Construct a new server using the supplied application factory. |
There was a problem hiding this comment.
suggestion: Include default worker count behavior in documentation
Please note in the documentation that the default worker count is determined by the number of CPU cores.
| /// Construct a new server using the supplied application factory. | |
| /// Construct a new server using the supplied application factory. | |
| /// | |
| /// By default, the number of worker threads is set to the number of CPU cores available. |
|
@coderabbitai issue: In WireframeServer::new you drop the .max(1) on the default worker count—consider adding .max(1) there as well to guarantee at least one worker, matching the behavior of the workers setter. |
|
Tip For best results, initiate chat on the files or code changes. @leynos Got it —this is a good catch. Would you like me to open a follow-up issue to track adding |
|
GitHub, please |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 954 @leynos The follow-up issue has been opened on GitHub ✅ Feel free to update the issue description or add any extra details as needed. 🐇 |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Consider preserving the previous max(1) guard in WireframeServer::new (e.g. num_cpus::get().max(1)) to ensure at least one worker on edge platforms.
- The Transform trait’s transform method could benefit from #[must_use] and #[inline] annotations similar to Next::new to encourage proper usage and performance.
- Since you’ve implemented From<Arc> and From for SharedState, consider deprecating SharedState::new(inner: Arc) to avoid API duplication and unify the constructor pattern.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider preserving the previous max(1) guard in WireframeServer::new (e.g. num_cpus::get().max(1)) to ensure at least one worker on edge platforms.
- The Transform trait’s transform method could benefit from #[must_use] and #[inline] annotations similar to Next::new to encourage proper usage and performance.
- Since you’ve implemented From<Arc<T>> and From<T> for SharedState, consider deprecating SharedState::new(inner: Arc<T>) to avoid API duplication and unify the constructor pattern.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Please create the following github issues:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2733 @leynos All three GitHub issues have been opened:
Note: The third issue’s title lost the back-ticked generics during shell quoting. If you’d like the title amended (e.g. “Deprecate Happy to help—just shout if anything needs tweaking! (_/) |
1 participant
Summary
Testing
markdownlint docs/roadmap.mdnixie docs/roadmap.mdcargo fmt --allcargo clippy -- -D warningscargo test -- --nocapturehttps://chatgpt.com/codex/tasks/task_e_684bd1f2b88483229b0c3a91dfddccc8
Summary by Sourcery
Fix compilation errors and resolve Clippy warnings by adding missing constructors, refining middleware and extractor traits, annotating public APIs, and updating documentation with a runnable example.
New Features:
WireframeServer::newconstructor to initialize server with a factoryNext::newconstructor in middleware to wrap servicesFrom<Arc<T>>andFrom<T>forSharedStateextractorEnhancements:
callandtransformAPIs#[must_use]and#[inline]annotations to key public methodsDocumentation:
docs/roadmap.mdTests:
implblock in extractor moduleSummary by CodeRabbit