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

Add brotlicffi support #6269

Merged
merged 8 commits into from
Mar 6, 2024
Merged

Add brotlicffi support #6269

merged 8 commits into from
Mar 6, 2024

Conversation

Laerte
Copy link
Member

@Laerte Laerte commented Mar 6, 2024

Fix #6263

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Merging #6269 (99f7165) into master (bf14935) will decrease coverage by 0.03%.
Report is 1 commits behind head on master.
The diff coverage is 50.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6269      +/-   ##
==========================================
- Coverage   88.91%   88.88%   -0.03%     
==========================================
  Files         161      161              
  Lines       11796    11802       +6     
  Branches     1914     1914              
==========================================
+ Hits        10488    10490       +2     
- Misses        964      968       +4     
  Partials      344      344              
Files Coverage Δ
scrapy/downloadermiddlewares/httpcompression.py 89.56% <50.00%> (-1.51%) ⬇️
scrapy/utils/_compression.py 89.61% <50.00%> (-2.29%) ⬇️

@Laerte Laerte requested review from wRAR and Gallaecio March 6, 2024 01:56
@wRAR
Copy link
Member

wRAR commented Mar 6, 2024

I guess we need an extra-deps job for PyPy?

@wRAR
Copy link
Member

wRAR commented Mar 6, 2024

Or no, we install brotli* for regular tests.

Co-authored-by: Andrey Rakhmatullin <wrar@wrar.name>
@Laerte
Copy link
Member Author

Laerte commented Mar 6, 2024

Or no, we install brotli* for regular tests.

Yeah I replaced the PyPy pinned version for this one, should I create extra-deps?

@Laerte Laerte requested a review from wRAR March 6, 2024 10:33
Comment on lines 13 to 15
brotli; implementation_name != 'pypy' # optional for HTTP compress downloader middleware tests
# 1.1.0 is broken on PyPy: https://github.com/google/brotli/issues/1072
brotli==1.0.9; implementation_name == 'pypy' # optional for HTTP compress downloader middleware tests
brotlicffi; implementation_name == 'pypy' # optional for HTTP compress downloader middleware tests
zstandard; implementation_name != 'pypy' # optional for HTTP compress downloader middleware tests
Copy link
Member

@Gallaecio Gallaecio Mar 6, 2024

Choose a reason for hiding this comment

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

Oh, so brotli and zstandard are here. It would be great if we could remove them from here and put them in the extra environments of tox.ini instead, to make sure Scrapy works without them. (this also applies to other packages here, but they are out of scope; in fact, zstandard is kind of out-of-scope, you could leave it here as well).

Co-authored-by: Adrián Chaves <adrian@chaves.io>
@Laerte Laerte requested a review from Gallaecio March 6, 2024 13:07
@wRAR wRAR merged commit 861646f into scrapy:master Mar 6, 2024
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add brotlicffi support
3 participants