Skip to content

Conversation

@cprevot93
Copy link

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.

@batfish-bot
Copy link

This change is Reviewable

@dhalperi
Copy link
Member

dhalperi commented Nov 7, 2025

Rad, but CI didn't pass. Can you fix it?

@cprevot93
Copy link
Author

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.

Copy link
Member

@dhalperi dhalperi left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @cprevot93)

Copy link
Member

@dhalperi dhalperi left a 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

@dhalperi
Copy link
Member

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.

Copy link
Author

@cprevot93 cprevot93 left a 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 | None they 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_kwargs type parameter that is a dict[str, any]? That would seem to perhaps be lower maintenance and more powerful

Yes, indeed!

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.

3 participants