Skip to content

add brotli as a dependency#31

Merged
BurnzZ merged 2 commits intomainfrom
add-brotli
Sep 20, 2022
Merged

add brotli as a dependency#31
BurnzZ merged 2 commits intomainfrom
add-brotli

Conversation

@BurnzZ
Copy link
Contributor

@BurnzZ BurnzZ commented Sep 19, 2022

Resolves #29

We don't have automated tests that's set up yet, but I've tested this manually that:

  • It sends 'Accept-Encoding': 'br' when brotli is installed.
  • It decompresses the response without any problem when 'Content-Encoding': 'br' is received.

@BurnzZ BurnzZ requested review from Gallaecio and kmike September 19, 2022 12:57
@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2022

Codecov Report

Merging #31 (eaf9208) into main (dd5e9f8) will not change coverage.
The diff coverage is 0.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #31   +/-   ##
=======================================
  Coverage   38.04%   38.04%           
=======================================
  Files          10       10           
  Lines         347      347           
  Branches       46       46           
=======================================
  Hits          132      132           
  Misses        215      215           
Impacted Files Coverage Δ
zyte_api/aio/client.py 27.39% <0.00%> (ø)

@kmike
Copy link
Collaborator

kmike commented Sep 19, 2022

Thanks @BurnzZ! The changes look good to me.

Before merging, could you please double-check that the responses are indeed compressed?

@BurnzZ
Copy link
Contributor Author

BurnzZ commented Sep 20, 2022

Before merging, could you please double-check that the responses are indeed compressed?

Manually confirmed that the responses are compressed. 👍

# adds direct client support for brotli has been released after 3.8.1:
# https://github.com/aio-libs/aiohttp/commit/28ea32d2282728a94af73c87efd6ab314c14320e
if HAS_BROTLI:
headers['Accept-Encoding'] = 'br'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be doing this even after that commit is released, i.e. we should always set Accept-Encoding to br, and not to gzip, deflate, br.

Also, since we are requiring brotli as a dependency, using HAS_BROTLI should not be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. 👍 Updated!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think previously it was fine as well - e.g. in an unlikely event that aiohttp's HAS_BROTLI logic would become different from "brotli library is installed", the code would still have worked, although with gzip instead of brotli.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to merge it though, it's not a big deal.

@BurnzZ BurnzZ merged commit 3966563 into main Sep 20, 2022
@wRAR wRAR deleted the add-brotli branch April 5, 2025 17:26
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 brotli to install_requires

4 participants