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

Implement Brotli support #2312

Merged
merged 11 commits into from
Oct 12, 2017
Merged

Implement Brotli support #2312

merged 11 commits into from
Oct 12, 2017

Conversation

oleksandr-kuzmenko
Copy link
Contributor

@oleksandr-kuzmenko oleksandr-kuzmenko commented Oct 10, 2017

What do these changes do?

Implement Brotli support

Are there changes in behavior for the user?

No.

Related issue number

#2270

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • Add a new news fragment into the changes folder

@asvetlov
Copy link
Member

Good start but tests are required.

@codecov-io
Copy link

codecov-io commented Oct 11, 2017

Codecov Report

Merging #2312 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2312      +/-   ##
==========================================
+ Coverage   97.23%   97.23%   +<.01%     
==========================================
  Files          39       39              
  Lines        8174     8179       +5     
  Branches     1430     1430              
==========================================
+ Hits         7948     7953       +5     
  Misses         98       98              
  Partials      128      128
Impacted Files Coverage Δ
aiohttp/http_parser.py 97.79% <100%> (+0.02%) ⬆️

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 cfed2f1...4a1ac01. Read the comment docs.

@oleksandr-kuzmenko oleksandr-kuzmenko changed the title [WIP] Implement Brotli support Implement Brotli support Oct 11, 2017
@oleksandr-kuzmenko
Copy link
Contributor Author

I hope this is not a big problem
Coverage 97.23% 97.18% -0.05%

@asvetlov
Copy link
Member

I pretty happy with the value.
Please add # pragma: no cover for lines which are executed if no brotlipy installed.

@hellysmile
Copy link
Member

Hello there, recently I was playing with exactly brotli and were using official python package.

Is there any difference between https://pypi.python.org/pypi/Brotli and https://pypi.python.org/pypi/brotlipy

Can we support both of them?

@oleksandr-kuzmenko
Copy link
Contributor Author

oleksandr-kuzmenko commented Oct 12, 2017

@hellysmile hello
I can't find docs for google brotli py-package =(

Brotlipy is a collection of bindings to the Brotli C library by Google, has docs and:

Brotlipy has a very similar interface to the standard library’s zlib module

I think no:

Can we support both of them?

@oleksandr-kuzmenko
Copy link
Contributor Author

Coverage 97.23% 97.23% +<.01%
FYI @asvetlov

Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Perfect, thank you
Looks like brotli and brotlipy have the same API

I'd like to have brotli compression support on both client and server sides but it's definitely a subject for other PRs

@asvetlov asvetlov merged commit 20854d6 into aio-libs:master Oct 12, 2017
@oleksandr-kuzmenko
Copy link
Contributor Author

In the near future I will try to add brotli support to server side

@asvetlov
Copy link
Member

I mean now we do support brotli decompression only, compressing requests/responses would be very useful too.

@oleksandr-kuzmenko
Copy link
Contributor Author

I got what you meant

@kxepal
Copy link
Member

kxepal commented Oct 12, 2017

awesome!

@hellysmile
Copy link
Member

I've tried to play around brotli\brotlipy, at least test is green on both of them.

@asvetlov
Copy link
Member

Looks like we need mention both libraries in doc

@oleksandr-kuzmenko
Copy link
Contributor Author

ok, i'll update docs


self.zlib = zlib.decompressobj(wbits=zlib_mode)
if encoding == 'br':
if not HAS_BROTLI: # pragma: no cover
Copy link
Member

Choose a reason for hiding this comment

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

Why this has no cover? Good coverage matters

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

brotlipy package could be added to extras in setup.py for usability reasons.

@asvetlov
Copy link
Member

We don't use extras in aiohttp (but I like the idea).
Hmm, we should mention brotly/brotlipy as optional dependency in index.rst and README.rst along with cchardet and aiodns.

@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bot:chronographer:provided There is a change note present in this PR outdated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants