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

change link for brotli package to a supported one #4267

Merged
merged 2 commits into from
Mar 17, 2022

Conversation

KristobalJunta
Copy link
Contributor

I have noticed that the offcial documentation suggests using brotlipy. While this works, the project seems to be stale or abandoned. Most recent release was in 2017 and most recent commit - in 2018.

I suggest replacing that with a more recent and seemingly more actively supported Brotli by google. It's most recent release was in 2018 and most recent commit - this month. This library has same API and judging by my experience, has proved to work seamlessly.

@codecov
Copy link

codecov bot commented Jan 4, 2020

Codecov Report

Merging #4267 (9062bb8) into master (3b42ccf) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4267   +/-   ##
=======================================
  Coverage   88.77%   88.77%           
=======================================
  Files         163      163           
  Lines       10639    10639           
  Branches     1813     1813           
=======================================
  Hits         9445     9445           
  Misses        919      919           
  Partials      275      275           

@kmike
Copy link
Member

kmike commented Jan 6, 2020

No strong opinion from me, but brotlipy doesn't look abandoned - there are recent discussions in pull requests. Google's brotli Python wrapper is also not changing much: if you check https://github.com/google/brotli/commits/master/python, a 2019 change is a comment added, and in 2018 there are 2 changes, one of which is test-only - this is not that different from brotlipy.

On the other hand, Google's version has a more recent brotli bundled.

If we suggest Google's library, we should change tests to run against it.

@KristobalJunta
Copy link
Contributor Author

@kmike yes, that's quite reasonable. Thank for the feedback!
I'll try and provide the tests for Google's implementation.

@KristobalJunta
Copy link
Contributor Author

@kmike I have updated requirements for tests and run them. Everything seems to pass.

@kmike
Copy link
Member

kmike commented Jan 20, 2020

@pawelmhm any opinions on brotli package, are you ok with changing it (as you implemented brotli support originally)?

@pawelmhm
Copy link
Contributor

I'm fine with that change @kmike let's merge this! Sorry for delay I completely missed this notification. It would be good to clean up brotli situation now, I think it should be ok to install it by default now, without making it optional dependency.

@Gallaecio
Copy link
Member

@KristobalJunta Do you think you will have time to resolve the current conflicts?

@KristobalJunta
Copy link
Contributor Author

@Gallaecio let me look into that and come back with update

Replace link to for brotli package in documentation to a supported implementattion.
This library has same API and has proved to work seamlessly.
@Gallaecio
Copy link
Member

Thank you! I think the failing jobs are unrelated to your changes. I’ll look into addressing #5425 and re-run the CI jobs here afterwards.

@KristobalJunta
Copy link
Contributor Author

@Gallaecio thank you! Looked into jobs myself and it seems as you said indeed.

@Gallaecio Gallaecio closed this Feb 23, 2022
@Gallaecio Gallaecio reopened this Feb 23, 2022
@Gallaecio Gallaecio merged commit 9a28eb0 into scrapy:master Mar 17, 2022
@Gallaecio
Copy link
Member

Thanks!

alexpdev pushed a commit to alexpdev/scrapy that referenced this pull request Apr 24, 2022
@KristobalJunta KristobalJunta deleted the update-brotli-link branch June 21, 2022 15:31
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.

None yet

4 participants