Skip to content

fix(client): correct handling of Sequence[Proxy] types#568

Merged
0x676e67 merged 1 commit intomainfrom
fix
Apr 14, 2026
Merged

fix(client): correct handling of Sequence[Proxy] types#568
0x676e67 merged 1 commit intomainfrom
fix

Conversation

@0x676e67
Copy link
Copy Markdown
Owner

@0x676e67 0x676e67 commented Apr 14, 2026

from: #564

Summary by CodeRabbit

  • Documentation

    • Clarified proxy configuration documentation for better understanding.
  • Performance

    • Applied compilation optimizations to improve efficiency.
  • Refactor

    • Restructured internal proxy handling architecture for improved maintainability.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 20762582-ff97-425c-a567-6496b5f21f77

📥 Commits

Reviewing files that changed from the base of the PR and between f2646e2 and 2a1769f.

📒 Files selected for processing (5)
  • python/wreq/wreq.py
  • src/client.rs
  • src/client/resp/http.rs
  • src/extractor.rs
  • src/macros.rs
✅ Files skipped from review due to trivial changes (2)
  • src/client/resp/http.rs
  • python/wreq/wreq.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/macros.rs
  • src/client.rs

📝 Walkthrough

Walkthrough

The PR refactors proxy configuration handling by removing the Extractor wrapper from proxy vectors. The type changes from Option<Extractor<Vec<Proxy>>> to Option<Vec<Proxy>>, requiring removal of the Python extraction implementation and corresponding adjustments to macro iteration logic.

Changes

Cohort / File(s) Summary
Docstring Update
python/wreq/wreq.py
Updated ClientConfig.proxies docstring from "Add a Proxy list to the client." to "The proxies to use for requests."
Proxy Handling Refactoring
src/client.rs, src/extractor.rs, src/macros.rs
Removed Extractor wrapper from proxy vectors. Imports adjusted, proxies field type simplified from Option<Extractor<Vec<Proxy>>> to Option<Vec<Proxy>>. Deleted FromPyObject implementation for Extractor<Vec<wreq::Proxy>>. Updated macro iteration logic to extract proxy from element's first component (item.0).
Optimization
src/client/resp/http.rs
Added #[inline] attribute to Response Drop implementation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

A rabbit hops through tangled types,
unwrapping proxies from their pipes,
now vectors flow direct and free,
from Python down to Rust decree! 🐰✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: removing improper extraction logic for proxy sequences and correcting the handling of Proxy types throughout the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors proxy handling by removing the Extractor wrapper for proxy sequences and updating the Client configuration to handle proxy vectors directly. Documentation and examples were updated to use tuples for proxies, and the Response drop implementation was optimized with an #[inline] attribute. Feedback focuses on removing commented-out code in examples for better clarity, correcting a docstring for pluralization, and addressing a maintenance concern regarding inconsistency in the updated set_if_some_iter_inner macro.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
docs/source/guide/proxy.md (1)

20-21: Remove commented legacy proxy line to avoid ambiguity.

Keeping both forms in-line (one commented) makes docs noisier and may imply one form is discouraged. Prefer one canonical snippet, and document alternatives in prose if needed.

🧹 Suggested doc cleanup
-        proxies=(Proxy.all("http://127.0.0.1:3067"),)
-        # proxies=[Proxy.http("socks5h://abc:[email protected]:6152")],
+        proxies=(Proxy.all("http://127.0.0.1:3067"),)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/source/guide/proxy.md` around lines 20 - 21, Remove the commented legacy
proxy line to avoid ambiguity: keep a single canonical example using the active
assignment (proxies=(Proxy.all("http://127.0.0.1:3067"),)) and delete the
commented Proxy.http example; if you need to show alternatives, add a short
prose sentence below describing the alternative use of Proxy.http/Proxy.socks
with credentials instead of leaving a commented code line.
examples/proxy.py (1)

9-10: Clean up commented legacy proxy config in the example.

This keeps the example focused and avoids mixed signals about preferred usage.

🧹 Suggested example cleanup
-        # proxies=[Proxy.http("socks5h://abc:[email protected]:6152")],
-        proxies=(Proxy.all("http://127.0.0.1:3067"),)
+        proxies=(Proxy.all("http://127.0.0.1:3067"),)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@examples/proxy.py` around lines 9 - 10, Remove the commented legacy proxy
line and any leftover commented examples (the line containing
Proxy.http("socks5h://abc:[email protected]:6152")) so the example only shows the
current preferred usage, i.e., the proxies assignment using
Proxy.all("http://127.0.0.1:3067"); update the proxies tuple (the proxies
variable) to be the single, uncommented entry and ensure no stray commented
proxy configurations remain in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@docs/source/guide/proxy.md`:
- Around line 20-21: Remove the commented legacy proxy line to avoid ambiguity:
keep a single canonical example using the active assignment
(proxies=(Proxy.all("http://127.0.0.1:3067"),)) and delete the commented
Proxy.http example; if you need to show alternatives, add a short prose sentence
below describing the alternative use of Proxy.http/Proxy.socks with credentials
instead of leaving a commented code line.

In `@examples/proxy.py`:
- Around line 9-10: Remove the commented legacy proxy line and any leftover
commented examples (the line containing
Proxy.http("socks5h://abc:[email protected]:6152")) so the example only shows the
current preferred usage, i.e., the proxies assignment using
Proxy.all("http://127.0.0.1:3067"); update the proxies tuple (the proxies
variable) to be the single, uncommented entry and ensure no stray commented
proxy configurations remain in the file.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d282ec69-154f-4c81-80be-c1443fdf193b

📥 Commits

Reviewing files that changed from the base of the PR and between 8eff131 and f2646e2.

📒 Files selected for processing (7)
  • docs/source/guide/proxy.md
  • examples/proxy.py
  • python/wreq/wreq.py
  • src/client.rs
  • src/client/resp/http.rs
  • src/extractor.rs
  • src/macros.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant