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

[MRG+1] fix a mistake in topic spider-middleware.rst #3201

Merged
merged 1 commit into from Jun 1, 2018

Conversation

@grammy-jiang
Copy link
Contributor

@grammy-jiang grammy-jiang commented Apr 5, 2018

The return iterable of process_spider_exception should contain scrapy.http.Request, not scrapy.http.Response.

@codecov
Copy link

@codecov codecov bot commented Apr 5, 2018

Codecov Report

Merging #3201 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3201   +/-   ##
=======================================
  Coverage   82.12%   82.12%           
=======================================
  Files         228      228           
  Lines        9593     9593           
  Branches     1385     1385           
=======================================
  Hits         7878     7878           
  Misses       1456     1456           
  Partials      259      259
@lopuhin
Copy link
Member

@lopuhin lopuhin commented Apr 5, 2018

This fix looks consistent with the docs, process_spider_exception docs say

If it returns an iterable the :meth:process_spider_output pipeline kicks in, and no other :meth:process_spider_exception will be called.

and process_spider_output docs say

:meth:process_spider_output must return an iterable of :class:~scrapy.http.Request, dict or :class:~scrapy.item.Item objects.
:param result: the result returned by the spider :type result: an iterable of :class:~scrapy.http.Request, dict or :class:~scrapy.item.Item objects

Although this is not checked in the code

@lopuhin lopuhin changed the title fix a mistake in topic spider-middleware.rst [MRG+1] fix a mistake in topic spider-middleware.rst Apr 5, 2018
@grammy-jiang
Copy link
Contributor Author

@grammy-jiang grammy-jiang commented Apr 5, 2018

Hi, @lopuhin

I have written several middlewares just about this method, process_spider_exception. They all work well but I have not checked the source code. The documents could be consistent between the classes of the spider and the spider middleware.

@lopuhin
Copy link
Member

@lopuhin lopuhin commented Apr 5, 2018

@grammy-jiang I agree - I think your fix is good and should be merged, I was just wondering what will happen if a response is returned.

@grammy-jiang
Copy link
Contributor Author

@grammy-jiang grammy-jiang commented Apr 5, 2018

@lopuhin Emmm... good point.

In my view, the method process_spider_output will be called when an iterable is returned from process_spider_exception, which means the thing will happen just like a response yielded from a spider.

So your question becomes what happens when a response yielded from a spider? I have no idea, no relative experience before...

@lopuhin
Copy link
Member

@lopuhin lopuhin commented Apr 5, 2018

Right, returning a response from a spider looks very strange indeed!

@grammy-jiang
Copy link
Contributor Author

@grammy-jiang grammy-jiang commented Apr 5, 2018

@lopuhin I have checked the class spidermw.py and I have not seen any result type assert here. The type of response will go through spider middleware without any exception.

So will there any type assert in engine or scheduler? I am not good enough at twisted, would you help to check? Thanks.

@kmike kmike merged commit 847b50c into scrapy:master Jun 1, 2018
2 checks passed
2 checks passed
codecov/patch Coverage not affected when comparing 6c3970e...cb76b88
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kmike kmike added this to the v1.6 milestone Jul 4, 2018
@kmike kmike modified the milestones: v1.6, v1.5.1 Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.