-
Notifications
You must be signed in to change notification settings - Fork 57
Add proxies and timeout parameters #949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Rad, but CI didn't pass. Can you fix it? |
|
Sorry, I'm having a hard time to run the test with uv. I still haven't test it, but hopefully it should be good now. |
dhalperi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhalperi reviewed 1 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @cprevot93)
dhalperi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @cprevot93)
pybatfish/client/session.py line 339 at r2 (raw file):
load_questions: bool = True, proxies: dict[str, str] | None = None, timeout: int = 30,
How does someone recover the default behavior of no timeout? Seems like without | None they can't?
pybatfish/client/session.py line 340 at r2 (raw file):
proxies: dict[str, str] | None = None, timeout: int = 30, ):
Thought question: should we just have a request_kwargs type parameter that is a dict[str, any]? That would seem to perhaps be lower maintenance and more powerful
|
Tests are green ,left 2 Qs for you. Feel free to ping me on Batfish slack. I got to these Qs because I was trying to write the release notes and realied I couldn't figure out how to undo the breaking change. |
cprevot93
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dhalperi)
pybatfish/client/session.py line 339 at r2 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
How does someone recover the default behavior of no timeout? Seems like without
| Nonethey can't?
Yes, you're right, but I don’t see any situation where you’d want to hang out indefinitely.
pybatfish/client/session.py line 340 at r2 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
Thought question: should we just have a
request_kwargstype parameter that is adict[str, any]? That would seem to perhaps be lower maintenance and more powerful
Yes, indeed!
Relay on requests features.
Proxies
Timeout
Timeout is important because: "By default, requests do not time out unless a timeout value is set explicitly. Without a timeout, your code may hang for minutes or more."
This change does not affect current codebase, except that it sets the connection timeout to 30 seconds. I doubt it will cause any code to break.