-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Add support for brotli decoding #5783
Conversation
Should that not be something more along the lines of... make_headers(accept_encoding=True).get('accept-encoding', ', '.join(('gzip', 'deflate'))), as a 'fail safe' |
You can change it as you want, as long Requests sends transparently .join is an overkiller to generate a constant string. |
My 2c: I like the approach of reusing code from urllib3 more than mine from #5554. :) However the minimal supported by requests version of urllib3 is now 1.21.1, which does not support brotli yet. The minimal version that has this support is 1.25.1. So I think that it should be mentioned in docs update here, along with info about required |
Such fail-safe is not needed, @VeNoMouS , as even the oldest supported by requests urllib3 has See https://github.com/urllib3/urllib3/blame/1.21.1/urllib3/util/request.py#L7-L55 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks mostly good, few comments for you. Also can we add a test for this?
When the brotli or brotlicffi packages are installed, urllib3.util.make_headers() inserts ',br' in the Accept-Encoding header and decodes br from the answers.
e44b721
to
f23c2e5
Compare
Please propose wording and where to update the text. Currently the brotli-texts are updated on three places - faq, history and quickstart. I consider it better, if the documentation does not repeat the same thing on different places. |
I apologize If this might come off as a bit rude but this PR seems to be the only thing blocking 2.26.0 milestone (the other changes in it have no pending change requested), and it's been a month without any progress, if for more time is needed that's of course understandable but would it be possible to bump this for a release after the next one so as not to have it block other changes from being released? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm now happy with my changes but I'd like another reviewer to take a quick peek now that I'm the author of most of this diff. cc @nateprewitt @sigmavirus24
This reverts commit 5351469.
* Add support for Brotli decoding When the brotli or brotlicffi packages are installed, urllib3.util.make_headers() inserts ',br' in the Accept-Encoding header and decodes br from the answers. * Create the default Accept-Encoding header once * Preserve the previous delimiter behavior * Update prose in quickstart.rst Co-authored-by: Seth Michael Larson <sethmichaellarson@gmail.com>
* Add support for Brotli decoding When the brotli or brotlicffi packages are installed, urllib3.util.make_headers() inserts ',br' in the Accept-Encoding header and decodes br from the answers. * Create the default Accept-Encoding header once * Preserve the previous delimiter behavior * Update prose in quickstart.rst Co-authored-by: Seth Michael Larson <sethmichaellarson@gmail.com>
* Add support for Brotli decoding When the brotli or brotlicffi packages are installed, urllib3.util.make_headers() inserts ',br' in the Accept-Encoding header and decodes br from the answers. * Create the default Accept-Encoding header once * Preserve the previous delimiter behavior * Update prose in quickstart.rst Co-authored-by: Seth Michael Larson <sethmichaellarson@gmail.com>
When the
brotli
orbrotlicffi
packages are installed,urllib3.util.make_headers()
inserts ',br' in the Accept-Encoding header and decodes br from the answers.