scrapinghub / scrapy-poet

Page Object pattern for Scrapy
BSD 3-Clause "New" or "Revised" License
119 stars 28 forks source link

Add tests that ensure transitive property of `instead_of` works. #90

Closed BurnzZ closed 1 year ago

BurnzZ commented 1 year ago

Stemming from: https://github.com/scrapinghub/web-poet/pull/84#discussion_r1004807507

@Gallaecio had brought up a good point:

I also wonder what happens when you have 2 levels of instead_of. If you have a @handle_urls decorator on B with instead_of=A, and on C with instead_of=B, is B used or is it C? Do we have a test for this?

Some tests should be written to ensure the expected behavior on this, as well as if the priority values are respected.

BurnzZ commented 1 year ago

Added a test in https://github.com/scrapinghub/scrapy-poet/pull/88.

Currently, there's no transitivity that happens for this: A -> B -> C. So if A is being required, C wouldn't be used as an override, but rather B.

There are use cases that merits the use of either behaviors. However, I think supporting transitive overrides makes the solution more complex. We could simply opt to create a rule that C overrides A to solve the problem.

kmike commented 1 year ago

I think not having such transitivity is fine. Let's close it.