Ensure proxies and timeout options are handled correctly#10
Merged
Conversation
sydneycollins-cio
approved these changes
Jun 29, 2026
| "timeout": timeout, | ||
| } | ||
|
|
||
| if proxies: |
There was a problem hiding this comment.
Non-blocking. proxies is only added to kwargs when truthy, so proxies={} silently drops it while the other options are always included. Either document that an empty dict means no proxy, or use if proxies is not None to be explicit.
Collaborator
Author
There was a problem hiding this comment.
OK will make this explicit
| def test_user_defined_upload_size(self): | ||
| client = Client('testsecret', on_error=self.fail, | ||
| upload_size=10, upload_interval=3) | ||
| upload_size=10, upload_interval=0.3) |
There was a problem hiding this comment.
Non-blocking. Is the upload_interval=3 → 0.3 change intentional? Looks unrelated to the proxy/timeout fix — if it is a test speed improvement, a comment would make it clear.
Collaborator
Author
There was a problem hiding this comment.
Will add comment, thanks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
proxiesoption on theClientconstructor was accepted but never actually passed to the underlyingrequestssession. Thepost()function inrequest.pybuilt akwargsdict containingproxies, but then called_session.post()with individual named arguments instead of using that dict._session.post(url, **kwargs)so all options (includingproxies) flow through.timeoutwas hardcoded to15in thekwargsdict instead of using thetimeoutparameter._session.postand assertproxiesis present in the call arguments.Test plan
test_proxies_passed_to_session— verifiesproxiesdict reaches_session.postviapost()test_no_proxies_by_default— verifiesproxiesis omitted when not setTestClient.test_proxies— verifiesproxiesflows end-to-end fromClientconstructor through to_session.postI made these changes.
🤖 Generated with Claude Code
Note
Medium Risk
Changes affect all batch uploads (timeout/proxy behavior); fixes are straightforward but touch the critical outbound HTTP path customers rely on.
Overview
HTTP upload options were silently ignored:
post()built akwargsdict withproxiesandtimeoutbut called_session.post()with separate arguments, soClientproxy settings never reached the wire. The call now uses_session.post(url, **kwargs), and the request kwargs use thetimeoutparameter instead of a hardcoded15.proxiesis only added when notNone.Proxy-related tests now mock
_session.postand assertproxiesis present or omitted; consumer proxy coverage checks stored config. Minor test tweaks addupload_intervalcoverage and stabilize batch timing.AGENTS.mddocuments opening PRs against customerio/cdp-analytics-python;CLAUDE.mdpoints agents at that file.Reviewed by Cursor Bugbot for commit 44600a0. Bugbot is set up for automated code reviews on this repo. Configure here.