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

Never compare bytes and str in putheader() #2141

Merged
merged 2 commits into from Jan 26, 2021

Conversation

@pquentin
Copy link
Member

@pquentin pquentin commented Jan 25, 2021

Fixes #2071. It's ugly, but I don't know how else to fix that.

I can't enable -bb before python-hyper/brotlicffi#177 is merged and released. (edit: disregard, there are other issues to fix before we can enable -bb.)

@codecov
Copy link

@codecov codecov bot commented Jan 25, 2021

Codecov Report

Merging #2141 (f0fb095) into main (d0b2076) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main     #2141   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         2216      2216           
=========================================
  Hits          2216      2216           
Impacted Files Coverage Δ
src/urllib3/connection.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0b2076...f0fb095. Read the comment docs.

@mgorny
Copy link
Contributor

@mgorny mgorny commented Jan 25, 2021

I agree it's ugly-ish but nevertheless kudos for finding some way to solve this. I've been pondering on it for some time and couldn't come up with anything even that good.

@pquentin
Copy link
Member Author

@pquentin pquentin commented Jan 25, 2021

Actually with the second commit it's not that bad! I wonder if I should add a comment.

Copy link
Member

@sethmlarson sethmlarson left a comment

This looks good, one thought:

@@ -184,7 +184,7 @@ def putrequest(self, method, url, *args, **kwargs):

def putheader(self, header, *values):
""""""
if SKIP_HEADER not in values:
if not any(isinstance(v, str) and v == SKIP_HEADER for v in values):
Copy link
Member

@sethmlarson sethmlarson Jan 25, 2021

Thoughts on using type(v) is str instead of isinstance()? Also can we use the -bb option as you've done in brotlicffi to ensure these error in the future?

Copy link
Contributor

@sigmavirus24 sigmavirus24 Jan 26, 2021

Why type(v) is str?

Copy link
Member Author

@pquentin pquentin Jan 26, 2021

As mentioned in the initial comment, I can't enable -bb because of brotlicffi (and other issues that I haven't investiaged yet).

I think isinstance() is the pythonic way to do this, and it performs just as well as type() is str (consistently less than 100nsec as mesured by timeit on my laptop).

Copy link
Member

@sethmlarson sethmlarson Jan 26, 2021

Was looking for a speed win in this often-called function :) It's all good though, let's merge this!

Copy link
Member

@sethmlarson sethmlarson left a comment

LGTM

@sethmlarson sethmlarson merged commit e47d3e7 into urllib3:main Jan 26, 2021
20 checks passed
@pquentin pquentin deleted the bytes-str-comparison branch Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants