Add get_generic_param() and fix finding Returns[] #184
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #184 +/- ##
==========================================
- Coverage 98.54% 98.40% -0.14%
==========================================
Files 30 29 -1
Lines 1378 1383 +5
==========================================
+ Hits 1358 1361 +3
- Misses 20 22 +2
|
| """Inherit from this generic mixin to change the item class used by | ||
| :class:`~.ItemPage`""" | ||
|
|
||
| @property | ||
| def item_cls(self) -> typing.Type[ItemT]: | ||
| def item_cls(self) -> type: |
There was a problem hiding this comment.
What's the reason for this change? Are there cases where a class which inherits from Returns[ItemType] doesn't return ItemType from item_cls?
There was a problem hiding this comment.
It can return a dict, when the page does not actually inherit from Returns[ItemType] but just from Returns[ItemT].
There was a problem hiding this comment.
It seems we should have added a couple of tests for that in https://github.com/scrapinghub/web-poet/blob/master/tests_typing/test_item_page.mypy-testing. We have the same issue with dict for other cases, like to_item. Also, Returns doesn't work properly if a type is changed in a subclass. But for a common case of not using dicts and not changing the type it should work correctly, as tests show; it seems this change might break it. I wonder if is it a right way to think about it or not :)
There was a problem hiding this comment.
We have a test that makes sure the method returns a dict:
Line 119 in 90eba4b
There was a problem hiding this comment.
Yes, I think that was the intention before the change (but no test in mypy-testing).
It's not a big deal overall, because this property should rarely be used by the end users though.
There was a problem hiding this comment.
We have a test that makes sure the method returns a dict
It's not a typing test (see https://github.com/scrapinghub/web-poet/tree/master/tests_typing); from the code point of view, it all should work fine.
There was a problem hiding this comment.
Isn't
about it?There was a problem hiding this comment.
Well, it and the next one (though I'm not sure why does the next one still pass).
There was a problem hiding this comment.
Yes, it's a right test; it is marked as xfail.
|
Let's merge it; I don't think typing discussion is important enough. |
|
Thanks @wRAR! |
Fixes #182 and will help in zytedata/zyte-common-items#49