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

Change defer.returnValue calls #4446

Closed
wants to merge 1 commit into from

Conversation

aditi137
Copy link
Contributor

@aditi137 aditi137 commented Mar 20, 2020

Changed all defer.returnValue calls in code and updated comments.
Resolves #4443

@wRAR
Copy link
Contributor

@wRAR wRAR commented Mar 20, 2020

Please run the tests locally before submitting the PR. As you can see on Travis, some tests failed, so some of your changes broke something.

defer.returnValue(response)
defer.returnValue((yield download_func(request=request, spider=spider)))
return response
yield download_func(request=request, spider=spider)
Copy link
Contributor

@wRAR wRAR Mar 20, 2020

Choose a reason for hiding this comment

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

The value is not returned here anymore

Copy link
Contributor

@joybh98 joybh98 Mar 24, 2020

Choose a reason for hiding this comment

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

@wRAR should return be used here?
As the function is ending there, there is no need to "save" the state. which yield is useful for

Copy link
Contributor

@wRAR wRAR Mar 24, 2020

Choose a reason for hiding this comment

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

Not sure what do you mean by "saving the state" but the function must return the result of that yield, just like it did before. yield here is used to get the result of download_func (because of @defer.inlineCallbacks).

Copy link
Contributor

@joybh98 joybh98 Mar 24, 2020

Choose a reason for hiding this comment

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

@wRAR okay 👍

I'm still a little bit confused by The value is not returned here anymore
What exactly is it not returning ?

Copy link
Contributor

@wRAR wRAR Mar 24, 2020

Choose a reason for hiding this comment

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

It is not returning the result of download_func.

Copy link
Contributor

@wRAR wRAR Mar 24, 2020

Choose a reason for hiding this comment

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

(which was previously returned via defer.returnValue as you can see)

Copy link
Contributor

@joybh98 joybh98 Mar 24, 2020

Choose a reason for hiding this comment

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

Okay 👍 I'm thinking of picking up this issue if the original author is not working on it.

@@ -124,8 +124,8 @@ def test_should_remove_req_res_references_before_caching_the_results(self):
# Simulate the Media Pipeline behavior to produce a Twisted Failure
try:
# Simulate a Twisted inline callback returning a Response
# The returnValue method raises an exception encapsulating the value
returnValue(response)
# The returned Response raises an exception
Copy link
Contributor

@wRAR wRAR Mar 20, 2020

Choose a reason for hiding this comment

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

It doesn't. defer.returnValue indeed raises an exception so it should either keep the test as is or change it, this depends on the test purpose. It may be related to the comment in MediaPipeline, or maybe not.

Copy link
Member

@elacuesta elacuesta Mar 21, 2020

Choose a reason for hiding this comment

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

+1 for rewriting the test. I think it'd be good to clean this block up, since the mentioned _DefGen_Return exception would not be in play anymore.

@@ -140,7 +140,7 @@ def test_should_remove_req_res_references_before_caching_the_results(self):

# The Failure should encapsulate a FileException ...
self.assertEqual(failure.value, file_exc)
# ... and it should have the returnValue exception set as its context
# ... and it should have the returned exception set as its context
Copy link
Contributor

@wRAR wRAR Mar 20, 2020

Choose a reason for hiding this comment

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

Same here (and in the similar comment below).

@@ -292,7 +292,7 @@ def crawl_log(self, spider):
crawler = get_crawler(spider)
with LogCapture() as log:
yield crawler.crawl(mockserver=self.mockserver)
raise defer.returnValue(log)
Copy link
Contributor

@wRAR wRAR Mar 20, 2020

Choose a reason for hiding this comment

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

I suspect the raise expression here was a mistake and it was ignored, so this should be rewritten as return log.

Copy link
Contributor

@joybh98 joybh98 Mar 24, 2020

Choose a reason for hiding this comment

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

@wRAR return log is leading to a lot of errors and test failures in my local machine.

Copy link
Contributor

@wRAR wRAR Mar 24, 2020

Choose a reason for hiding this comment

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

@joybhallaa it doesn't on mine.

Copy link
Contributor

@joybh98 joybh98 Mar 24, 2020

Choose a reason for hiding this comment

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

That's cool, just thought I'd let you know.

Copy link
Contributor

@wRAR wRAR Mar 24, 2020

Choose a reason for hiding this comment

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

@joybhallaa that probably means you changed something else.

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.

4 participants