feat(client): implement raise_for_status for Client#583
Conversation
📝 WalkthroughWalkthroughAdds a client-level ChangesError Status Control Feature
Sequence Diagram(s)sequenceDiagram
participant PyClient as Python Client
participant RustClient as Rust Client
participant Executor as execute_request
participant HTTP as HTTP Backend
PyClient->>RustClient: create Client (raise_for_status=?)
PyClient->>RustClient: request(...)
RustClient->>Executor: execute_request(request, client.clone())
Executor->>HTTP: send HTTP request
HTTP-->>Executor: HTTP response
alt client.raise_for_status == true
Executor->>Executor: response.error_for_status() -- may raise
else client.raise_for_status == false
Executor-->>Executor: keep response as-is
end
Executor-->>RustClient: Response / Error
RustClient-->>PyClient: Response / Error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client.rs (1)
459-466:⚠️ Potential issue | 🔴 CriticalFix compilation error:
configis out of scope.
configonly exists inside theif let Some(mut config) = kwdsblock (lines 263-457), but this code reads it at line 465 after that block has ended. Capture the flag whileconfigis still in scope, or assign it to a local before building theClient.🔧 Suggested fix
fn new(py: Python, kwds: Option<Builder>) -> PyResult<Client> { py.detach(|| { // Create the client builder. let mut builder = wreq::Client::builder(); let mut cookie_jar: Option<Jar> = None; + let mut raise_for_status = false; if let Some(mut config) = kwds { // Emulation options. apply_option!(set_if_some, builder, config.emulation, emulation); @@ // Compression options. apply_option!(set_if_some, builder, config.gzip, gzip); apply_option!(set_if_some, builder, config.brotli, brotli); apply_option!(set_if_some, builder, config.deflate, deflate); apply_option!(set_if_some, builder, config.zstd, zstd); + + raise_for_status = config.raise_for_status.unwrap_or(false); } builder .build() .map(|inner| Client { inner, cancel: CancellationToken::new(), cookie_jar, - raise_for_status: config.raise_for_status.unwrap_or(false), + raise_for_status, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client.rs` around lines 459 - 466, The build closure references `config` out of scope when constructing the Client (raise_for_status: config.raise_for_status.unwrap_or(false)); fix by capturing the flag while `config` is still in scope—e.g., inside the `if let Some(mut config) = kwds` block compute a local boolean like `let raise_for_status = config.raise_for_status.unwrap_or(false)` (or otherwise store it in a local variable) and then use that local (`raise_for_status`) in the `builder.build().map(|inner| Client { ... raise_for_status, })` call so no out-of-scope variable is accessed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/client.rs`:
- Around line 459-466: The build closure references `config` out of scope when
constructing the Client (raise_for_status:
config.raise_for_status.unwrap_or(false)); fix by capturing the flag while
`config` is still in scope—e.g., inside the `if let Some(mut config) = kwds`
block compute a local boolean like `let raise_for_status =
config.raise_for_status.unwrap_or(false)` (or otherwise store it in a local
variable) and then use that local (`raise_for_status`) in the
`builder.build().map(|inner| Client { ... raise_for_status, })` call so no
out-of-scope variable is accessed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 990f984a-b462-4a14-93c6-aea10504ef1b
📒 Files selected for processing (3)
python/wreq/wreq.pysrc/client.rssrc/client/req.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/req.rs`:
- Around line 419-425: The closure currently only checks client.raise_for_status
and ignores the per-request override; change it to compute the effective flag by
reading Request.raise_for_status (from where it was extracted at Line 237) and
falling back to client.raise_for_status when the request value is not set, then
call r.error_for_status() only when that effective flag is true; update the
closure that currently references client.raise_for_status to use this computed
effective value instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c2f2cc0-3b27-4028-8a1e-12507c1ab0c1
📒 Files selected for processing (2)
src/client.rssrc/client/req.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client.rs
|
This commit only implements functionality similar to setting |
Fix #581
Summary by CodeRabbit