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] Support for returning deferreds in middlewares #1473

Merged
merged 7 commits into from Sep 2, 2015
Merged

[MRG+1] Support for returning deferreds in middlewares #1473

merged 7 commits into from Sep 2, 2015

Conversation

@ArturGaspar
Copy link
Contributor

@ArturGaspar ArturGaspar commented Sep 1, 2015

Adds support for returning deferreds in middlewares, and makes use of this to fix a limitation in RobotsTxtMiddleware.
#1471

@codecov-io
Copy link

@codecov-io codecov-io commented Sep 1, 2015

Current coverage is 82.33%

Merging #1473 into master will increase coverage by +0.09% as of 6e0cc5e

Powered by Codecov. Updated on successful CI builds.

@@ -52,9 +58,19 @@ def robot_parser(self, request, spider):
meta={'dont_obey_robotstxt': True}
)
dfd = self.crawler.engine.download(robotsreq, spider)
dfd.addCallback(self._parse_robots)
dfd.addCallback(self._parse_robots, netloc)

This comment has been minimized.

@dangra

dangra Sep 2, 2015
Member

👍

@dangra
Copy link
Member

@dangra dangra commented Sep 2, 2015

LGTM 👏s

@dangra dangra changed the title Support for returning deferreds in middlewares [MRG+1] Support for returning deferreds in middlewares Sep 2, 2015
@dangra
Copy link
Member

@dangra dangra commented Sep 2, 2015

the patch looks amazing, if it can get 100% coverage on robotstxt middleware it would be awesome :)

https://codecov.io/github/scrapy/scrapy/scrapy/downloadermiddlewares/robotstxt.py?ref=f34d8e309e723e5cc54a892d0c2e58ccd6e01a23

@dangra
Copy link
Member

@dangra dangra commented Sep 2, 2015

@ArturGaspar thanks for increasing coverage of robotstxt mw, looks like it didn't reach 100% because of a missing branch check at https://codecov.io/github/scrapy/scrapy/scrapy/downloadermiddlewares/robotstxt.py?ref=af2acd5046c9349b4f1082b401bf8a8a9f9d93b9#l-76.

I saw you added deferred support to spidermiddlewares but please, keep this PR focused on adding support to downloader middleware, It is acceptable to port robotstxt becaues it is a good example on how to use this feature.

Spider Middleware have subtles differences like been able to return iterables so I prefer if its support is added in another PR.

@dangra dangra changed the title [MRG+1] Support for returning deferreds in middlewares [MRG] Support for returning deferreds in middlewares Sep 2, 2015
@ArturGaspar
Copy link
Contributor Author

@ArturGaspar ArturGaspar commented Sep 2, 2015

How do I remove the commit for the spider middleware changes from the pull request?
Will I have to close it and make another one? Or should I revert the changes in another commit?

@dangra
Copy link
Member

@dangra dangra commented Sep 2, 2015

How do I remove the commit for the spider middleware changes from the pull request?

use git rebase --interactive locally and remove the commit then git push --force to the same branch.

Will I have to close it and make another one? Or should I revert the changes in another commit?

No, no need to create another PR.
Reverting by adding another commit is fine but at the end you will have to squash your commits before we merge.

@ArturGaspar
Copy link
Contributor Author

@ArturGaspar ArturGaspar commented Sep 2, 2015

100% coverage for robotstxt.py now.

@dangra
Copy link
Member

@dangra dangra commented Sep 2, 2015

100% coverage for robotstxt.py now.
Hide all checks

cool! how much for 100% coverage in all lines affected by this patch? ;)

@ArturGaspar
Copy link
Contributor Author

@ArturGaspar ArturGaspar commented Sep 2, 2015

I'm not really sure this is the best way to test that part, but it needs to be covered so there it is.

@dangra

This comment has been minimized.

Copy link
Member

@dangra dangra commented on tests/test_downloadermiddleware.py in 2748b38 Sep 2, 2015

what do you think if instead of checking response type it checks the for response instance identity.

I mean, create a Response object in test case scope, return it from process_request() and check it is the same response with self.assertIs() in this line.

@ArturGaspar
Copy link
Contributor Author

@ArturGaspar ArturGaspar commented Sep 2, 2015

It's a better idea indeed.

@dangra dangra changed the title [MRG] Support for returning deferreds in middlewares [MRG+1] Support for returning deferreds in middlewares Sep 2, 2015
dangra added a commit that referenced this pull request Sep 2, 2015
[MRG+1] Support for returning deferreds in middlewares
@dangra dangra merged commit dd47314 into scrapy:master Sep 2, 2015
2 checks passed
2 checks passed
codecov/patch 100.00% of diff hit (target 100.00%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dangra
Copy link
Member

@dangra dangra commented Sep 2, 2015

thanks!

@chekunkov
Copy link
Contributor

@chekunkov chekunkov commented Sep 18, 2015

yeah! finally someone got it done! 👍

@pablohoffman
Copy link
Member

@pablohoffman pablohoffman commented Sep 18, 2015

Indeed! 👏

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

Successfully merging this pull request may close these issues.

None yet

5 participants