Skip to content
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

Rework the HTTPS connection classes #1730

Closed
pquentin opened this issue Nov 3, 2019 · 1 comment
Closed

Rework the HTTPS connection classes #1730

pquentin opened this issue Nov 3, 2019 · 1 comment

Comments

@pquentin
Copy link
Member

pquentin commented Nov 3, 2019

We realized in #1726 that all HTTPS connections go through the VerifiedHTTPSConnection class, even when they're unverified. This means we can merge VerifiedHTTPSConnection into HTTPSConnection.

However, those classes are part of our public API (see https://urllib3.readthedocs.io/en/latest/reference/index.html#module-urllib3.connection). The current layout is:

  • HTTPSConnection is not visible, but is exported as UnverifiedHTTPSConnection
  • VerifiedHTTPSConnection is available directly, but is also exported as HTTPSConnection

To preserve compatibility, we can expose HTTPSConnection directly, and then also export it as VerifiedHTTPSConnection and UnverifiedHTTPSConnection. This allows us to move to a single class while still exposing the same classes as before.

It will actually be an improvement, because HTTPSConnection exported as UnverifiedHTTPSConnection currently does not work as its connect() method uses variables that don't exist.

Another option is to stop exporting aliases, and only expose HTTPSConnection, but this is a breaking change. For example, it's likely to break vcrpy: https://github.com/kevin1024/vcrpy/blob/7caf29735ac6bae9fd17310460a00d66c3eb3a95/vcr/stubs/requests_stubs.py. So let's not do this.

(It would be interesting to make sure that vcr.py still works after this change.)

@pquentin
Copy link
Member Author

pquentin commented Feb 9, 2020

Closing in favor of #1799.

@pquentin pquentin closed this as completed Feb 9, 2020
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

No branches or pull requests

1 participant