-
Notifications
You must be signed in to change notification settings - Fork 4
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
SERP (MVP) #62
SERP (MVP) #62
Conversation
…ctual implementation
With the serp item released, what are the obstacles in adding Serp support to scrapy-zyte-api? (anyways, let's handle it separately, it's not blocking anything) |
I’ve just opened scrapy-plugins/scrapy-zyte-api#218 out of local changes I had started. There is no blocker, but it is not entirely a trivial change, it is not very complex either, but the special behavior of serp compared to other data types does complicate things more than usual, and there are other Zyte API features that are not covered in scrapy-zyte-api yet that we should also cover while adding serp support. |
@BurnzZ Any quick thought on the crawl logging changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Gallaecio , I just tested and reviewed the changes pertaining to the crawling logs. All looks well. 👍
Left a few minor comments as well. Aside from that, LGTM!
Release notes added, I’ll handle the release as soon as we merge. |
Pagination is currently hard-coded to 10, which is not always accurate I believe. However, the next iteration of SERP extraction will include a nextPage entry, so I think it is best to start with this simple approach and switch to a nextPage-based implementation later.
This implementation does not rely on scrapy-poet because scrapy-zyte-api does not yet support
serp
properly. To be changed once we add proper support for it there (working on it).To do:
setup.py
accordingly.