Improve follow/follow_all #41
Conversation
MatthewZMSU
added
documentation
scrapypuppeteer/response.py
Outdated
|
|
||
| class PuppeteerHtmlResponse(PuppeteerResponse): | ||
| # Probably, we should ban any PuppeteerAction in `actions` except GoTo |
There was a problem hiding this comment.
I agree. I believe we can even forbid any non-navigable actions in PuppeteerRequest if there is no page id.
scrapypuppeteer/response.py
Outdated
| page_id = self.page_id # Substitution of page_id in order to create new page | ||
| self.page_id = None | ||
|
|
||
| yield from ( | ||
| self.follow( | ||
| action, | ||
| close_page=close_page, | ||
| accumulate_meta=accumulate_meta, | ||
| **kwargs, | ||
| ) | ||
| for action in actions | ||
| ) | ||
|
|
||
| self.page_id = page_id |
There was a problem hiding this comment.
I dislike this pattern because it modifies the response during iteration and allows constructions like
requests_iter = response.follow_all(links)
assert response.page_id is not None
next_request = next(requests_iter, None)
assert response.page_id is None
I suggest either passing page_id=None to self.follow, or collecting following requests to list first.
There was a problem hiding this comment.
First option is not possible right now because in self.follow we determine page_id as page_id = None if self.puppeteer_request.close_page else self.page_id.
I modified my solution so changing self.page_id inside follow_all method is not visible to user.
Still, it modifies the response during iteration. I like the way of providing page_id=None to self.follow but currently this raises an error. Probably, it's a bug.
I suggest to solve the problem within the PR related to this comment. We will properly validate arguments not only in the service but also in the client part.
Description
In this PR I have improved
followmethod ofPuppeteerResponseclass.Also I have added
follow_allmethod in it.Updated documentation.
Type of change
How Has This Been Tested?
Tested it with several local spiders.
They worked fine.
Checklist: