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
[MRG+1] [httpcompression] add support for br - brotli content encoding #2535
Conversation
Does the library require anything special/additional to get installed properly? |
I didnt have to install anything, but it seems like some environments are failing to install, so most likely some systems are missing required libraries. Testing for support at runtime may be much safer yes, I'll add that @redapple |
Codecov Report@@ Coverage Diff @@
## master #2535 +/- ##
==========================================
- Coverage 83.49% 83.49% -0.01%
==========================================
Files 161 161
Lines 8787 8795 +8
Branches 1289 1290 +1
==========================================
+ Hits 7337 7343 +6
- Misses 1203 1205 +2
Partials 247 247
Continue to review full report at Codecov.
|
@pawelmhm , I do not see brotlipy being installed for Python 3.x tests on Travis. |
@@ -1,23 +1,33 @@ | |||
import zlib | |||
|
|||
|
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.
pep8 says 1 line between import groups :)
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.
good point, we could add flake8 style checks for PR. Flake8 could run on diff and point out all things like this.
tests/requirements.txt
Outdated
@@ -6,6 +6,7 @@ pytest==2.9.2 | |||
pytest-twisted | |||
pytest-cov==2.2.1 | |||
jmespath | |||
brotlipy==0.6 |
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'd prefer not to pin brotlipy version here: users will be installing it with 'pip install brotlipy', and we won't be able to detect if a brotlipy upgrade broke scrapy if we pin a version in tests.
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.
updated fb4ef21
The PR looks good, besides a couple of minor points |
@kmike re performance there are good benchmarks and charts here: https://quixdb.github.io/squash-benchmark/#ratio-vs-compression |
@pawelmhm I was more worried about Python wrappers speed |
brotli encoding is used by multiple websites, e.g. amazon started to use it recently. Support in Scrapy is added via python library brotlipy https://github.com/python-hyper/brotlipy/