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

Support for delegated methods as callbacks #4756

merged 3 commits into from Aug 25, 2020


Copy link

@ivanprado ivanprado commented Aug 25, 2020

It is useful to split the spider code around some helper classes if the spider code is too big. Using inheritance and mixings is the regular approach in this case. But using callback delegation could be a useful alternative in some cases but Scrapy does not support it. For example, this is not allowed currently:

class UKHomepageParser:
  def parse(self, response):

class USHomepageParser:
  def parse(self, response):

class MySpider(Spider):
  def __init__(self, location):
    if location == "UK:
      self.parse = UKHomepageParser().parse
      self.parse = USHomepageParser().parse


Nothing prevents Scrapy to support this case. This PR changes the function _find_method in removing the constraint that forces a callback self to be the spider. Being a method from a different class is not a problem as long as this method is a member of spider class. The required change to support this is pretty small.

It can be useful to structure the spiders code around some helper classes.
Copy link

@Gallaecio Gallaecio left a comment

Looks good to me.

@kmike kmike added this to the 2.4 milestone Aug 25, 2020
@Gallaecio Gallaecio self-requested a review Aug 25, 2020
Copy link

Gallaecio commented Aug 25, 2020

This needs a slight update:

Or maybe it does not. I mean, they still need to be methods of the Spider class, it’s only that now they can be methods that delegate on the methods of a different object.

Copy link

codecov bot commented Aug 25, 2020

Codecov Report

Merging #4756 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4756   +/-   ##
  Coverage   87.17%   87.17%           
  Files         160      160           
  Lines        9815     9810    -5     
  Branches     1447     1446    -1     
- Hits         8556     8552    -4     
  Misses        996      996           
+ Partials      263      262    -1     
Impacted Files Coverage Δ
scrapy/utils/ 94.59% <100.00%> (+1.73%) ⬆️
scrapy/core/downloader/handlers/ 93.03% <0.00%> (ø)

Thanks @victor-torres for the suggestion
Copy link

@victor-torres victor-torres left a comment

LGTM! 👏🏻 Great work, @ivanprado!

Copy link

kmike commented Aug 25, 2020

Makes sense, thanks @ivanprado!

@kmike kmike merged commit 0ccaf89 into scrapy:master Aug 25, 2020
2 checks passed
Copy link
Contributor Author

ivanprado commented Aug 26, 2020

Thank you @Gallaecio @victor-torres and @kmike for the review. Very quick! 😄

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

Successfully merging this pull request may close these issues.

None yet

4 participants