Initial support for typing.Annotated, support for extractFrom #141
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #141 +/- ##
==========================================
- Coverage 98.96% 98.64% -0.33%
==========================================
Files 10 10
Lines 776 810 +34
==========================================
+ Hits 768 799 +31
- Misses 8 11 +3
|
scrapy_zyte_api/providers.py
Outdated
| for option in ExtractFrom: | ||
| if option in metadata: | ||
| product_options = zyte_api_meta.setdefault("productOptions", {}) | ||
| if "extractFrom" in product_options: |
There was a problem hiding this comment.
This doesn't check if the previous value is the same, maybe we should do that instead.
There was a problem hiding this comment.
Alternatively, we should treat values from different sources with different priorities and use the top one instead of raising this error.
| @attrs.define | ||
| class AnnotatedProductPage(BasePage): | ||
| product: Annotated[Product, ExtractFrom.httpResponseBody] | ||
| product2: Annotated[Product, ExtractFrom.httpResponseBody] |
There was a problem hiding this comment.
This doesn't raise a double-extractFrom exception because there is only one unique dep.
| @@ -117,6 +117,14 @@ def render_POST(self, request): | |||
| "price": "10", | |||
| "currency": "USD", | |||
| } | |||
| extract_from = request_data.get("productOptions", {}).get("extractFrom") | |||
| if extract_from: | |||
| from scrapy_zyte_api.providers import ExtractFrom | |||
There was a problem hiding this comment.
Alternatively it can be moved to a different module (importing providers needs additional deps).
| assert isinstance(response_data["product"], dict) | ||
| assert isinstance(response_data["product"]["name"], str) |
There was a problem hiding this comment.
These are needed because of how response_data is typed, otherwise mypy thinks the values could be anything supported by JSON.
| product2: Annotated[Product, ExtractFrom.httpResponseBody] | ||
|
|
||
| class AnnotatedZyteAPISpider(ZyteAPISpider): | ||
| def parse_(self, response: DummyResponse, page: AnnotatedProductPage): # type: ignore[override] |
There was a problem hiding this comment.
error: Argument 2 of "parse_" is incompatible with supertype "ZyteAPISpider"; supertype defines the argument type as "ProductPage" [override]
note: This violates the Liskov substitution principle
note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
I think this is a general problem with scrapy-poet-annotated callbacks, it will also be worse when we release typed Scrapy as Scrapy.parse has Response but we often use DummyResponse.
There was a problem hiding this comment.
Could you please open an issue for this in scrapy-poet?
There was a problem hiding this comment.
I will, but I'm not sure what to do with it :)
There was a problem hiding this comment.
wRAR
changed the title
|
So the last question is where should we define |
|
What about a |
Requires scrapinghub/andi#25 and scrapinghub/scrapy-poet#169