Skip to content

Fix HTTPConnection timeout type #3565

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 5, 2020
Merged

Fix HTTPConnection timeout type #3565

merged 1 commit into from
Jan 5, 2020

Conversation

dfarley1
Copy link
Contributor

HTTPConnection.timeout is passed down to socket.settimeout() which supports Optional[float] and has a specific action for None.

@dfarley1
Fix HTTPConnection timeout type
210b0a1 
`HTTPConnection` only passes timeout down to `socket.settimeout()` which is of type `Optional[float]` and has a specific action for `None`.  `HTTPConnection` should support the same behavior
@bluetech
Copy link
Contributor

bluetech commented Jan 4, 2020

The HTTPConnection doc doesn't mention this option of passing None to timeout, so the resulting behavior ("the socket is put in blocking mode") might just be coincidental.

Do you have a use case for it? If yes, it would also be good to send a PR to Python to document it.

srittau
srittau approved these changes Jan 5, 2020
View reviewed changes
Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

LGTM, since we follow implementation over documentation. That said, it would be good if this was documented either way in the http docs.

@srittau srittau merged commit 4fb4c80 into python:master Jan 5, 2020
@dfarley1
Copy link
Contributor Author

dfarley1 commented Jan 5, 2020

LGTM, since we follow implementation over documentation. That said, it would be good if this was documented either way in the http docs.

Awesome! I created python/cpython#17843 to get it documented, let's see where that goes.

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

@srittau srittau srittau approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dfarley1 @bluetech @srittau