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

On better handling use cases of PEP-380 inside request callbacks #3484

Closed
starrify opened this issue Nov 8, 2018 · 4 comments · Fixed by #3869
Closed

On better handling use cases of PEP-380 inside request callbacks #3484

starrify opened this issue Nov 8, 2018 · 4 comments · Fixed by #3869

Comments

@starrify
Copy link
Contributor

starrify commented Nov 8, 2018

Recently I observed people writing something like this (when using Python 3.3+):

# coding: utf8

import scrapy


class TestSpider(scrapy.Spider):
    name = 'test'
    start_urls = ['http://httpbin.org/get']

    def parse(self, response):
        yield scrapy.Request('http://httpbin.org/get?foo=bar')
        return {'url': response.url}

That won't work as expected as the return <some value> statement here, as defined in PEP-380, is just equivalent to raise StopIteration(<some value>).
Sample job logs: link

Currently (as of dc65e75) Scrapy simply ignores the "returned" value, which might be causing confusions to users. Sample job log of the code above: link

A proposal is to generate an error (or at least warning) when such cases happen (StopIteration observed to have an value).

@lopuhin
Copy link
Member

lopuhin commented Nov 10, 2018

Another option would be to capture returned value as if it was yielded, I'm not sure how to implement it though.

@starrify
Copy link
Contributor Author

starrify commented Nov 10, 2018

Thanks @lopuhin for the idea. I'm sorry that I'm against it.

The "returned" values (or to be precise, the feature of returning values via the StopIteration exception) as introduced in PEP-380 are not for yielding values from the generator, that's for returning a value from subgenerators to the delegating generator.
Sample code and the result: link

I don't doubt at all that we're technically able to somehow intercept the process and force such values to be picked by Scrapy, just as normal values from the generator. But that doesn't look like the right thing to do to me.

@starrify
Copy link
Contributor Author

starrify commented Nov 10, 2018

Also, from PEP-380 it has got this as one of the rejected ideas:

Suggestion: If close() is not to return a value, then raise an exception if StopIteration with a non-None value occurs.

While I agree that it is something we shall not have at the language level, I think it beneficial to check for such cases for only request callbacks within the scope of Scrapy, as I assume such mistakes may have been bothering some people for some time already.

@baby5
Copy link

baby5 commented Nov 12, 2018

this is right, not necessary
there are many things like this in scrapy👐

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants