Skip to content
This repository has been archived by the owner on Apr 14, 2022. It is now read-only.

Rename inject_into_urllib3/extract_from_urllib3 #171

Merged
merged 2 commits into from
Nov 30, 2019

Conversation

pquentin
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Nov 30, 2019

Codecov Report

Merging #171 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #171   +/-   ##
=======================================
  Coverage   99.49%   99.49%           
=======================================
  Files          28       28           
  Lines        1995     1995           
=======================================
  Hits         1985     1985           
  Misses         10       10

@njsmith
Copy link
Member

njsmith commented Nov 30, 2019

Does this code even work on hip? It seems like it requires intimate knowledge of the underlying I/O system...

@pquentin
Copy link
Member Author

@njsmith Oh, no, this does not work at all and caused us problems in the past. This is also the first reason why we don't support atlernate TLS backends. So the new names still don't work. But I figured it wasn't a reason to keep the old names.

@sethmlarson
Copy link
Contributor

If the code doesn't work should we get rid of the inject functions? Or are we planning for them to work?

@pquentin
Copy link
Member Author

We need to find a way to make the functionality work without mutating sys.modules. But I don't know how to do that! Should I leave the old name for the time being, or is the rename still valuable even though those functions don't work?

@njsmith
Copy link
Member

njsmith commented Nov 30, 2019

I opened an issue to discuss the substantive questions about these modules future, so it doesn't end up collecting on a minor search/replace PR :-) #177

I guess for this PR itself.... I think it's quite possible that we'll delete this code entirely, but I take that as an argument for merging the PR. The PR's not making anything worse, it lets us decouple the name update work from the TLS library support work, and we're going to revisit this anyway so let's not get hung up here.

@njsmith njsmith merged commit 4b23971 into python-trio:master Nov 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants