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 ZYTE_API_ENABLED #40

Closed
wants to merge 7 commits into from

Conversation

Gallaecio
Copy link
Contributor

@Gallaecio Gallaecio commented Aug 12, 2022

Resolves #12

@Gallaecio Gallaecio requested review from kmike and BurnzZ August 12, 2022 08:40
@codecov
Copy link

codecov bot commented Aug 12, 2022

Codecov Report

Merging #40 (a4706f0) into main (e06219d) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
+ Coverage   99.31%   99.34%   +0.03%     
==========================================
  Files           3        3              
  Lines         145      152       +7     
==========================================
+ Hits          144      151       +7     
  Misses          1        1              
Impacted Files Coverage Δ
scrapy_zyte_api/handler.py 99.02% <100.00%> (+0.07%) ⬆️

README.rst Outdated
}
}
The ``ZYTE_API_ENABLED`` setting can be used to control whether all, none, or
some requests go through Zyte Data API. It supports the following values:
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be better to have a separate option for enabling all requests to go through Zyte API, because of the following reasons:

  1. Usually ..._ENABLED options just turn on/off the middleware.
  2. It may also be tricky to set this setting to None from the command line, if I recall correctly.
  3. We shoud consider future Zyte API features, with data extraction use cases: if user only wants the extraction features, they'd need to set ZYTE_API_ENABLED to None, which is a bit strange.
  4. Instead of relying on the user to specify correct request parameters to use Zyte API as a downloader, it'd be nice to set them on our side. This may involve not only passing static default parameters, but also taking request method, body and headers in account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3 is not fully clear to me, but I don’t think it matters at this point.

Shall we deal with 4 on a follow-up pull request, or here? I would love to work on it, but I also would like to discuss the API a bit before working on an implementation.

README.rst Outdated Show resolved Hide resolved
@Gallaecio
Copy link
Contributor Author

Feel free to suggest alternatives to ZYTE_API_ALL (ZYTE_API_ALL_REQUESTS? ZYTE_API_FORCE? ZYTE_API_USE_BY_DEFAULT?)

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
tests/test_api_requests.py Outdated Show resolved Hide resolved
tests/test_api_requests.py Outdated Show resolved Hide resolved
tests/test_api_requests.py Show resolved Hide resolved
Copy link
Member

@BurnzZ BurnzZ left a comment

Choose a reason for hiding this comment

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

LGTM @Gallaecio !

@Gallaecio
Copy link
Contributor Author

Closing in favor of #41

@Gallaecio Gallaecio closed this Sep 20, 2022
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.

Ability to set the default Zyte API Parameters for every Request
3 participants