Recursive injection with an example

[ivanprado]

Having recursive dependency injection can help to create smaller PageObject components that extracts only parts of a page, and then define compounded PageObjects with several of them. See example books_06.

The example of books_06 presents boilerplate. A lot of yield from self. and response repeated and repeated.

@kmike It would be nice if you can have a look and give your opinion about all that.

I was not able to check if cyclic dependency detection is working
because I failed to create an example. The following fails with the error NameError: name 'B' is not defined in method get_type_hints:

@attr.s(auto_attribs=True)
class A(WebPage):
    b: 'B'

@attr.s(auto_attribs=True)
class B(WebPage):
    a: A

[codecov]

Codecov Report

Merging #2 into master will decrease coverage by 4.86%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##           master       #2      +/-   ##
==========================================
- Coverage   63.91%   59.04%   -4.87%     
==========================================
  Files           6        6              
  Lines          97      105       +8     
==========================================
  Hits           62       62              
- Misses         35       43       +8

Impacted Files	Coverage Δ
scrapy_po/middleware.py	30.76% <14.28%> (-7.95%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0776e7...fd191a4. Read the comment docs.

[kmike]

I wonder if a problem with get_type_hints can be solved using https://docs.python.org/3/whatsnew/3.7.html#pep-563-postponed-evaluation-of-annotations, though I kind-of expected it to work as-is.

[kmike]

Recursive injection makes total sense to me, I think that'd be a good feature to have. It is listed as "imitation of the implementation" in https://gist.github.com/kmike/2fa2b1b8c77bfb98f72b2636e66c5d36#dependency-injection, having a very similar application in mind.

[kmike]

Any ideas about a better API? Inheriting "page part" component from a WebPage looks a bit strange, on a first sight

[ivanprado]

Yes is strange. Also, all the "pieces" are incorporating the selector API without sharing it, which I don't like very much. Well, I have to think about it. This is very preliminary to me, I just noticed that I needed the recursive injection and started to experiment with it.

[kmike]

Also, all the "pieces" are incorporating the selector API without sharing it, which I don't like very much.

Sorry, could you please explain it more?

[kmike]

If the goal of inheriting is to be able to say "please do dependency injection for this class", it could make sense to have a base class for this (e.g. Component, or something like this - this is becoming Java pretty fast), and have PageObject inherit from Component.

[kmike]

Aha, maybe I know what you mean. In the code I was showing you on a call (which is not commited), selector-specific logic is moved to a mixin:

class PageObject(abc.ABC):
    """
    All web page objects should inherit from this class.
    It is a marker for a middleware to pick anargument and populate it.

    Instead of inheriting you can also use ``PageObject.register(MyWebPage)``.
    """
    pass


class ItemPage(PageObject):
    """ Page Object which requires to_item method to be implemented. """
    @abc.abstractmethod
    def to_item(self):
        pass


class ResponseShortcutsMixin:
    """
    A mixin with common shortcut methods for working with self.response.
    It requires "response" attribute to be present.
    """
    _cached_selector = None

    @property
    def selector(self) -> parsel.Selector:
        from parsel.selector import Selector
        if self._cached_selector is None:
            self._cached_selector = Selector(self.html)
        return self._cached_selector

    @property
    def url(self):
        return self.response.url

    @property
    def html(self):
        return self.response.html

    def xpath(self, query, **kwargs):
        return self.selector.xpath(query, **kwargs)

    def css(self, query):
        return self.selector.css(query)


@attr.s(auto_attribs=True)
class WebPage(PageObject, ResponseShortcutsMixin):
    """
    Default WebPage class. It uses basic response data (html, url),
    and provides xpath / css shortcuts.

    This class is most useful as a base class for page objects, when
    extraction logic is similar to what's usually happening in scrapy
    spider callbacks.
    """
    response: ResponseData

Let me asend a PR with these changes.

[kmike]

#3 - could you please check it & merge if it makes things easier?

[ivanprado]

Thanks @kmike ! I left some comments in the PR itself that I think covers yoursquestions.

[ivanprado]

Thank @kmike for all your feedback. A quick sight to the get_type_hints problems reveals that the globals that get_type_hints use at the constructor method are limited to the class itself so module globals are not included (and they should). Probably andi should be changed to also include the module in globals. Could you give me rights in andi to propose PR requests?

[ivanprado]

It seems that the problem with the type is related to an issue in attrs: python-attrs/attrs#593. I'll create a xfail test in andi for it.

[ivanprado]

This has been implemented in #2 in a different way, probably better. And the problem with get_type_hints was addressed in andi. So closing.