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

Drop urllib3 in favor of public gist #1182

Merged
merged 11 commits into from
Sep 4, 2020
Merged

Drop urllib3 in favor of public gist #1182

merged 11 commits into from
Sep 4, 2020

Conversation

florimondmanca
Copy link
Member

Fixes #1143

This PR removes the URLLib3Transport from HTTPX, and moves it to a public gist that we're now pointing to in the "Custom transports" docs.

Also adds a small additional section to the Requests compatibility guide, so that people know this option exists if they're migrating from Requests to HTTPX.

Copy link
Member

@jcugat jcugat left a comment

Choose a reason for hiding this comment

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

Changes look good. We can remove the --omit=httpx/_transports/urllib3.py from scripts/coverage now.

@tomchristie
Copy link
Member

Fantastic! Let's hold off on pulling in API changes until we're ready to roll a major release. We could consider maintaining a release branch?

@florimondmanca florimondmanca added api change PRs that contain breaking public API changes do not merge PRs that should not be merged labels Aug 17, 2020
@tomchristie
Copy link
Member

The only other tiny tweak I can think of here in the gist would be changing from this:

        except (urllib3.exceptions.SSLError, socket.error) as exc:
            tb = exc.__traceback__
            raise httpcore.NetworkError(exc).with_traceback(tb) from None

To this:

        except (urllib3.exceptions.SSLError, socket.error) as exc:
            raise httpcore.NetworkError(exc) from exc

Which I think gets us the traceback behaviour we actually.

https://gist.github.com/florimondmanca/d56764d78d748eb9f73165da388e546e#file-urllib3_transport-py-L101

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

Okay, it's got a do-not-merge label on, which makes sense, but otherwise yup this is ace.
So, approved.

Do we think we ought to be maintaining a version branch so that we can pull this in, and then maintain the version branch until we're ready to go?

@tomchristie tomchristie mentioned this pull request Sep 2, 2020
@tomchristie
Copy link
Member

In the gist, let's just use raise httpcore.NetworkError(exc) from exc styles, instead of .with_traceback(tb), in these two cases:

Once that's tweaked let's get this merged!

docs/compatibility.md Outdated Show resolved Hide resolved
@tomchristie
Copy link
Member

I'd suggest we also include a link in the "third party packages" docs.

@florimondmanca
Copy link
Member Author

florimondmanca commented Sep 4, 2020

let's just use raise httpcore.NetworkError(exc) from exc styles, instead of .with_traceback(tb), in these two cases

I'd suggest we also include a link in the "third party packages" docs.

Also dropped the recommendation to use urllib3-transport while migrating — I reckon that's not something even I might not actually do if I were to migrate a Requests-based project to use HTTPX.

Edit: also updated the README and docs home page to not mention urllib3 as an optional dependency anymore (da4a456).

@florimondmanca florimondmanca removed the do not merge PRs that should not be merged label Sep 4, 2020

<!-- NOTE: this list is in alphabetical order. -->

### urllib3-transport
Copy link
Member Author

Choose a reason for hiding this comment

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

Not too keen to put this under "Plugins", since it's not really an installable package, but then also not keen on the existing "Plugins" section — perhaps we ought to just list everything as h2-level headers. We can address this as a follow-up.

@florimondmanca florimondmanca merged commit 8fa8765 into master Sep 4, 2020
@florimondmanca florimondmanca deleted the fm/urllib3-gist branch September 4, 2020 20:56
@tomchristie
Copy link
Member

Fantastic. 🌟

@tomchristie tomchristie mentioned this pull request Sep 21, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change PRs that contain breaking public API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move URLLib3Transport out of core and into a stand-alone package.
4 participants