Supporting to_return in web-poet rules

[BurnzZ]

In line with the upcoming development in scrapinghub/web-poet#84.

This is built on top of #89 for now which moves away from the deprecated functionalities of web-poet as introduced in scrapinghub/web-poet#84.

This also addresses #90 .

TODO:

• move away from deprecated web-poet functionalities
• tests and adjustments for the new to_return parameter in web-poet's rules.
  • callback_for
  • add test cases when multiple rules for same PO is supported (update RulesRegistry to allow multiple rules in a single Page Object web-poet#101)
• deprecate passing tuples in SCRAPY_POET_OVERRIDES and the Registry with the advent of to_return.
• pass mypy
• update setup.py and tox.ini to point to the new web-poet version containing new registry features.

NOTES:

• A drop in code coverage has been observed since I moved most of test utilities into scrapy_poet/utilities/ (reference) which did not have any tests to begin with. We can add tests to them in a separate PR.
• Some other things we could do, but they could be placed into another PR to make this cleaner.
  • consider renaming OverridesRegistry into RulesRegistry
  • consider renaming SCRAPY_POET_OVERRIDES into SCRAPY_POET_RULES
  • consider renaming SCRAPY_POET_OVERRIDES_REGISTRY into SCRAPY_POET_RULES_REGISTRY
  • UPDATE: PR created in Docs and deprecations after supporting to_return=Item #103
• We could also move most of the URL matching functionality of the scrapy-poet's registry into web-poet. Let's do this in another PR.
  • UPDATE: web-poet PR in move some registry functionalities from scrapy-poet web-poet#112
• This doesn't include the functionality of selecting which fields to populate when asking for items. This warrants its own API and would be discussed in a separate PR.
  • UPDATE: PR WIP: Annotated[Item, PickFields("x", "y")] to decide which fields to populate in callback #111
• Documentation and tutorial for selecting items in POs and callbacks could be placed in the other PR. We can even include it in the PR for selecting which fields to populate (see bullet point above).
  • UPDATE: PR created in Docs and deprecations after supporting to_return=Item #103

[codecov]

Codecov Report

Merging #88 (1caa0de) into master (399371f) will decrease coverage by 3.05%.
The diff coverage is 100.00%.

❗ Current head 1caa0de differs from pull request most recent head 140239a. Consider uploading reports for the commit 140239a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #88      +/-   ##
==========================================
- Coverage   91.91%   88.87%   -3.05%     
==========================================
  Files          13       14       +1     
  Lines         569      737     +168     
==========================================
+ Hits          523      655     +132     
- Misses         46       82      +36     

Impacted Files	Coverage Δ
scrapy_poet/commands.py	47.05% <ø> (ø)
scrapy_poet/utils/mockserver.py	67.44% <ø> (ø)
scrapy_poet/api.py	100.00% <100.00%> (ø)
scrapy_poet/downloadermiddlewares.py	100.00% <100.00%> (ø)
scrapy_poet/injection.py	98.97% <100.00%> (ø)
scrapy_poet/injection_errors.py	100.00% <100.00%> (ø)
scrapy_poet/page_input_providers.py	94.91% <100.00%> (-1.14%)	⬇️
scrapy_poet/utils/__init__.py	100.00% <100.00%> (ø)
scrapy_poet/utils/testing.py	81.57% <100.00%> (ø)
... and 2 more

[BurnzZ]

This should return a more apt exception since UndeclaredProvidedTypeError primarily means that there's no provider available to give the requested dependency (in this case, the item).

However, this scenario is mostly due to an incorrect class that was returned.

Perhaps a better result is raising MalformedProvidedClassesError or creating another exception like IncorrectProvidedClassError.

[wRAR]

Continuing the today's discussion: my current implementation of a Scrapy command that takes a PO and returns its dependencies and result (like crawl_item_and_deps() from this PR, but integrated into the scrapy.cmdline-provided crawler) uses the following things from this PR: tests.test_web_poet_rules.spider_for(), tests.utils.create_scrapy_settings() + tests.test_web_poet_rules.rules_settings(), tests.utils.CollectorPipeline, tests.utils.InjectedDependenciesCollectorMiddleware. Its implementation looks like this:

        po = load_object(po_name)
        # here we could also support running a specific spider to apply its custom_settings etc
        spider_cls = spider_for(po)
        self.settings.setdict(additional_settings())
        crawler = Crawler(spider_cls, self.settings)
        self.crawler_process.crawl(crawler, url=url)
        self.crawler_process.start()
        items = crawler.spider.collected_items[0]
        deps = crawler.spider.collected_response_deps[0]

So if we keep this implementation, it would need at least CollectorPipeline, InjectedDependenciesCollectorMiddleware and spider_for somewhere in scrapy_poet, not in its tests.

[BurnzZ]

Hi @wRAR , I moved most of the test utilities in scrapy_poet/utils/ so we could reuse most of it in the package. 9a00b63

However, I haven't moved spider_for() and rules_settings() since they're quite specific to the type of tests they're used. There are a few variations of this as well. For these, I think we can redefine them in the context we're using them.

[kmike]

👍 🎉 🚀

[kmike]

Any idea what's up with the CI @BurnzZ? I tried to restart jobs, but it didn't help. No logs, nothing :)