Skip to content

Backport start_tls() support for Python 3.6#521

Merged
florimondmanca merged 2 commits intomasterfrom
start-tls-3.6
Nov 17, 2019
Merged

Backport start_tls() support for Python 3.6#521
florimondmanca merged 2 commits intomasterfrom
start-tls-3.6

Conversation

@sethmlarson
Copy link
Contributor

This adds support for proxy tunnels for Python 3.6 + asyncio.
Based almost entirely on this comment: urllib3/urllib3#1323 (comment) Massive thanks to @florimondmanca for finding this!

Closes #515.

@sethmlarson sethmlarson requested a review from a team November 11, 2019 02:01
@lovelydinosaur
Copy link
Contributor

Fantastic stuff!

I guess this PR should also update the test_start_tls_on_socket_stream test case, right?

Copy link
Contributor

@yeraydiazdiaz yeraydiazdiaz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @sethmlarson! 🌟

Copy link
Contributor

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Awesome! Approving in advance provided we update the test mentioned by Tom. :) Thanks!

@lovelydinosaur
Copy link
Contributor

At a later date I may look at moving this out to a coverage-excluded compat.py module, so that we can tweak our tests to expect 100% coverage on any single run, without requiring collection across multiple Python versions. (Buit I'm very happy to treat that seperately)

Comment on lines +66 to +67
if hasattr(loop, "start_tls"):
loop_start_tls = loop.start_tls # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing the type: ignore after changing the if statement to be if sys.version_info >= (3, 7) should pass CI with mypy>=0.730.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately we can't assume that loops on Python 3.6 don't have start_tls(). Uvloop supports 3.6 and supports start_tls(). That's interesting that mypy knows about features though! Would an assert work here?

Copy link
Contributor

Choose a reason for hiding this comment

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

getattr would work around any typing issues too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I didn't know about uvloop 😞

That's interesting that mypy knows about features though!

It knows about if/else with sys.version_info comparisons but that's all. It's very limited.

Would an assert work here?

I don't think we can assert much about the function at runtime, unfortunately. Could maybe do something with typing.cast but probably not worth the effort.

Copy link
Contributor

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

I removed the version check for 3.6 on the start_tls test, all tests passing!

Mental note that we should refactor sometime in the future to have a compat.py file with this kind of version-specific patching.

Anyway, merging now, thanks @sethmlarson!

@florimondmanca florimondmanca merged commit 331be99 into master Nov 17, 2019
@florimondmanca florimondmanca deleted the start-tls-3.6 branch November 17, 2019 11:50
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.

HTTPX claims Python 3.6+ support when asyncio backend TCPStream.start_tls explicitly does not support Python 3.6

5 participants