Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add get_generic_param() and fix finding Returns[] #184

Merged
merged 1 commit into from
Aug 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion tests/test_pages.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import List, Optional
from typing import Generic, List, Optional, TypeVar

import attrs
import pytest
Expand Down Expand Up @@ -232,6 +232,31 @@ class SubclassStrict(BasePage, Returns[Item]):
await page2.to_item()


def test_returns_inheritance() -> None:
@attrs.define
class MyItem:
name: str

class BasePage(ItemPage[MyItem]):
@field
def name(self):
return "hello"

MetadataT = TypeVar("MetadataT")

class HasMetadata(Generic[MetadataT]):
pass

class DummyMetadata:
pass

class Page(BasePage, HasMetadata[DummyMetadata]):
pass

page = Page()
assert page.item_cls is MyItem


@pytest.mark.asyncio
async def test_extractor(book_list_html_response) -> None:
@attrs.define
Expand Down
78 changes: 76 additions & 2 deletions tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,17 @@
import inspect
import random
import warnings
from typing import Any
from typing import Any, Generic, TypeVar
from unittest import mock

import pytest

from web_poet.utils import _create_deprecated_class, cached_method, ensure_awaitable
from web_poet.utils import (
_create_deprecated_class,
cached_method,
ensure_awaitable,
get_generic_param,
)


class SomeBaseClass:
Expand Down Expand Up @@ -466,3 +471,72 @@ async def n_called(self):
foo.n_called(),
)
assert results == [1, 1, 1, 1, 1]


ItemT = TypeVar("ItemT")


class Item:
pass


class Item2:
pass


class MyGeneric(Generic[ItemT]):
pass


class MyGeneric2(Generic[ItemT]):
pass


class Base(MyGeneric[ItemT]):
pass


class BaseSpecialized(MyGeneric[Item]):
pass


class BaseAny(MyGeneric):
pass


class Derived(Base):
pass


class Specialized(BaseSpecialized):
pass


class SpecializedAdditionalClass(BaseSpecialized, Item2):
pass


class SpecializedTwice(BaseSpecialized, Base[Item2]):
pass


class SpecializedTwoGenerics(MyGeneric2[Item2], BaseSpecialized):
pass


@pytest.mark.parametrize(
["cls", "param"],
[
(MyGeneric, None),
(Base, None),
(BaseAny, None),
(Derived, None),
(BaseSpecialized, Item),
(Specialized, Item),
(SpecializedAdditionalClass, Item),
(SpecializedTwice, Item2),
(SpecializedTwoGenerics, Item),
],
)
def test_get_generic_param(cls, param) -> None:
assert get_generic_param(cls, expected=MyGeneric) == param
24 changes: 0 additions & 24 deletions web_poet/_typing.py

This file was deleted.

33 changes: 26 additions & 7 deletions web_poet/pages.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
import abc
import inspect
import typing
from contextlib import suppress
from functools import wraps
from typing import Any, Generic, Optional, TypeVar, overload

import attr
import parsel

from web_poet._typing import get_item_cls
from web_poet.fields import FieldsMixin, item_from_fields
from web_poet.mixins import ResponseShortcutsMixin, SelectorShortcutsMixin
from web_poet.page_inputs import HttpResponse
from web_poet.utils import CallableT, _create_deprecated_class, cached_method
from web_poet.utils import (
CallableT,
_create_deprecated_class,
cached_method,
get_generic_param,
)


class Injectable(abc.ABC, FieldsMixin):
Expand All @@ -35,25 +39,40 @@
Injectable.register(type(None))


def is_injectable(cls: typing.Any) -> bool:
def is_injectable(cls: Any) -> bool:
"""Return True if ``cls`` is a class which inherits
from :class:`~.Injectable`."""
return isinstance(cls, type) and issubclass(cls, Injectable)


ItemT = typing.TypeVar("ItemT")
ItemT = TypeVar("ItemT")


class Returns(typing.Generic[ItemT]):
class Returns(Generic[ItemT]):
"""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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or was it not working properly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can return a dict, when the page does not actually inherit from Returns[ItemType] but just from Returns[ItemT].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a test that makes sure the method returns a dict:

assert page.item_cls is dict

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't

reveal_type(item) # R: dict
about it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it and the next one (though I'm not sure why does the next one still pass).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a right test; it is marked as xfail.

"""Item class"""
return get_item_cls(self.__class__, default=dict)


@overload
def get_item_cls(cls: type, default: type) -> type:
...

Check warning on line 63 in web_poet/pages.py

View check run for this annotation

Codecov / codecov/patch

web_poet/pages.py#L63

Added line #L63 was not covered by tests


@overload
def get_item_cls(cls: type, default: None) -> Optional[type]:
...

Check warning on line 68 in web_poet/pages.py

View check run for this annotation

Codecov / codecov/patch

web_poet/pages.py#L68

Added line #L68 was not covered by tests


def get_item_cls(cls: type, default: Optional[type] = None) -> Optional[type]:
param = get_generic_param(cls, Returns)
return param or default


_NOT_SET = object()


Expand Down
3 changes: 1 addition & 2 deletions web_poet/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,8 @@
import attrs
from url_matcher import Patterns, URLMatcher

from web_poet._typing import get_item_cls
from web_poet.page_inputs.url import _Url
from web_poet.pages import ItemPage
from web_poet.pages import ItemPage, get_item_cls
from web_poet.utils import _create_deprecated_class, as_list, str_to_pattern

Strings = Union[str, Iterable[str]]
Expand Down
25 changes: 24 additions & 1 deletion web_poet/utils.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import inspect
import weakref
from collections import deque
from collections.abc import Iterable
from functools import lru_cache, partial, wraps
from types import MethodType
from typing import Any, Callable, List, Optional, TypeVar, Union
from typing import Any, Callable, List, Optional, Tuple, TypeVar, Union, get_args
from warnings import warn

import packaging.version
Expand Down Expand Up @@ -273,3 +274,25 @@ def str_to_pattern(url_pattern: Union[str, Patterns]) -> Patterns:
if isinstance(url_pattern, Patterns):
return url_pattern
return Patterns([url_pattern])


def get_generic_param(
cls: type, expected: Union[type, Tuple[type, ...]]
) -> Optional[type]:
"""Search the base classes recursively breadth-first for a generic class and return its param.

Returns the param of the first found class that is a subclass of ``expected``.
"""
visited = set()
queue = deque([cls])
while queue:
node = queue.popleft()
visited.add(node)
for base in getattr(node, "__orig_bases__", []):
origin = getattr(base, "__origin__", None)
if origin and issubclass(origin, expected):
result = get_args(base)[0]
if not isinstance(result, TypeVar):
return result
queue.append(base)
return None