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.

Sign up for GitHub

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

introduce @field(disabled=True) #120

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

introduce @field(disabled=True) #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e39e4d
introduce @field(disable=True)
BurnzZ Jan 30, 2023
e76d42e
prevent _FIELDS_INFO_ATTRIBUTE_READ from being mutated
BurnzZ Jan 31, 2023
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
110 changes: 109 additions & 1 deletion tests/test_fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import asyncio
import random
from typing import Optional

import attrs
import pytest
Expand All @@ -26,7 +27,7 @@
item_from_fields,
item_from_fields_sync,
)
from web_poet.fields import get_fields_dict
from web_poet.fields import FieldInfo, get_fields_dict


@attrs.define
Expand Down Expand Up @@ -522,3 +523,110 @@ def b(self):
return None

assert set(get_fields_dict(B)) == {"a", "b", "mixin"}


@pytest.mark.asyncio
async def test_field_disabled() -> None:
@attrs.define
class Item:
x: int
y: Optional[int] = None
z: Optional[int] = None

class Page(ItemPage[Item]):
@field
def x(self) -> int:
return 1

@field(disabled=False)
def y(self) -> int:
return 2

@field(disabled=True)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe enabled would be a better name.

I would prefer something that suggests how the field can still be used as a property but is not included in the to_item output, but I cannot think of a good name (export? itemize? itemizable? to_item? add_to_item? include_in_item?).

Copy link
Contributor Author

@BurnzZ BurnzZ Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My initial implementation actually used enabled, but it seems get_fields_dict(include_disabled=True) sounds much better.

Hmmm, to_item and include_in_item does sound better in terms of what it does. Currently, either enabled or disabled isn't that clear about what it does.

+1 on something like @field(to_item=False).

Alongside with this, perhaps renaming to get_fields_dict(all_fields=True) would work.

Copy link
Member

Choose a reason for hiding this comment

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

I was initially thinking about something like "exclude_by_default=True" or "excluded_by_default=True", or "extract_by_default=False"; alternatives like "to_item=False" also look fine.

A possible issue with to_item=False is that you may think such field doesn't have to be defined in the item, but it does; I think it's still an error if item doesn't have it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was initially thinking about something like "exclude_by_default=True" or "excluded_by_default=True", or "extract_by_default=False"

I think these have the same issue with include and exclude where it's not clear what's happening, i.e., where it's being excluded from.

A possible issue with to_item=False is that you may think such field doesn't have to be defined in the item, but it does; I think it's still an error if item doesn't have it.

You're right. What do you think about having something like omit_in_to_item=True or to_item_omit=True?

def z(self) -> int:
return 3

page = Page()
assert await page.to_item() == Item(x=1, y=2)
assert page.x == 1
assert page.y == 2
assert page.z == 3

fields_dict_instance = get_fields_dict(page)
fields_dict_class = get_fields_dict(Page)

for info in [fields_dict_class, fields_dict_instance]:
assert info["x"] == FieldInfo(name="x", meta=None, out=None, disabled=False)
assert info["y"] == FieldInfo(name="y", meta=None, out=None, disabled=False)

fields_dict_instance = get_fields_dict(page, include_disabled=True)
fields_dict_class = get_fields_dict(Page, include_disabled=True)

for info in [fields_dict_class, fields_dict_instance]:
assert info["x"] == FieldInfo(name="x", meta=None, out=None, disabled=False)
assert info["y"] == FieldInfo(name="y", meta=None, out=None, disabled=False)
assert info["z"] == FieldInfo(name="z", meta=None, out=None, disabled=True)

# The subclass should properly reflect any changes to the ``disable`` value

class SubPage(Page):
"""Flicks the switch for ``y`` and ``z``."""

@field(disabled=True)
def y(self) -> int:
return 2

@field(disabled=False)
def z(self) -> int:
return 3

subpage = SubPage()
assert await subpage.to_item() == Item(x=1, z=3)
assert subpage.x == 1
assert subpage.y == 2
assert subpage.z == 3

fields_dict_instance = get_fields_dict(subpage)
fields_dict_class = get_fields_dict(SubPage)

for info in [fields_dict_class, fields_dict_instance]:
assert info["x"] == FieldInfo(name="x", meta=None, out=None, disabled=False)
assert info["z"] == FieldInfo(name="z", meta=None, out=None, disabled=False)

fields_dict_instance = get_fields_dict(subpage, include_disabled=True)
fields_dict_class = get_fields_dict(SubPage, include_disabled=True)

for info in [fields_dict_class, fields_dict_instance]:
assert info["x"] == FieldInfo(name="x", meta=None, out=None, disabled=False)
assert info["y"] == FieldInfo(name="y", meta=None, out=None, disabled=True)
assert info["z"] == FieldInfo(name="z", meta=None, out=None, disabled=False)

# Disabling fields that are required in the item cls would error out.

class BadSubPage(Page):
@field(disabled=True)
def x(self) -> int:
return 1

badsubpage = BadSubPage()

with pytest.raises(TypeError):
await badsubpage.to_item()

assert badsubpage.x == 1
assert badsubpage.y == 2
assert badsubpage.z == 3

fields_dict_instance = get_fields_dict(badsubpage)
fields_dict_class = get_fields_dict(BadSubPage)

for info in [fields_dict_class, fields_dict_instance]:
assert info["y"] == FieldInfo(name="y", meta=None, out=None, disabled=False)

fields_dict_instance = get_fields_dict(badsubpage, include_disabled=True)
fields_dict_class = get_fields_dict(BadSubPage, include_disabled=True)

for info in [fields_dict_class, fields_dict_instance]:
assert info["x"] == FieldInfo(name="x", meta=None, out=None, disabled=True)
assert info["y"] == FieldInfo(name="y", meta=None, out=None, disabled=False)
assert info["z"] == FieldInfo(name="z", meta=None, out=None, disabled=True)
56 changes: 47 additions & 9 deletions web_poet/fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@
_FIELDS_INFO_ATTRIBUTE_WRITE = "_web_poet_fields_info_temp"


def _fields_template():
return {"enabled": {}, "disabled": {}}
Copy link
Contributor Author

@BurnzZ BurnzZ Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

An alternative approach is to declare something like:

_FIELDS_INFO_ATTRIBUTE_READ = "_web_poet_fields_info"
_FIELDS_INFO_ATTRIBUTE_WRITE = "_web_poet_fields_info_temp"
_FIELDS_INFO_ATTRIBUTE_READ_DISABLED = "_web_poet_fields_info_disabled"
_FIELDS_INFO_ATTRIBUTE_WRITE_DISABLED = "_web_poet_fields_info_temp_disabled"

But I think it could be unwieldly when more field-flags are introduced later on.

We could also simply use the same arrangement as before but then get_fields_dict() would need to loop over all FieldInfo instances and check which are disabled or not.



@attrs.define
class FieldInfo:
"""Information about a field"""
Expand All @@ -29,6 +33,9 @@ class FieldInfo:
#: field processors
out: Optional[List[Callable]] = None

#: when set to ``True``, the field is not populated on ``.to_item()`` calls.
disabled: bool = False


class FieldsMixin:
"""A mixin which is required for a class to support fields"""
Expand All @@ -39,11 +46,31 @@ def __init_subclass__(cls, **kwargs):
# between subclasses, i.e. a decorator in a subclass doesn't affect
# the base class. This is done by making decorator write to a
# temporary location, and then merging it all on subclass creation.
this_class_fields = getattr(cls, _FIELDS_INFO_ATTRIBUTE_WRITE, {})
base_class_fields = getattr(cls, _FIELDS_INFO_ATTRIBUTE_READ, {})
this_class_fields = getattr(
cls, _FIELDS_INFO_ATTRIBUTE_WRITE, _fields_template()
)
base_class_fields = getattr(
cls, _FIELDS_INFO_ATTRIBUTE_READ, _fields_template()
)
if base_class_fields or this_class_fields:
fields = {**base_class_fields, **this_class_fields}
setattr(cls, _FIELDS_INFO_ATTRIBUTE_READ, fields)
enabled = {**base_class_fields["enabled"], **this_class_fields["enabled"]}
for name in this_class_fields["disabled"]:
if name in enabled:
del enabled[name]

disabled = {
**base_class_fields["disabled"],
**this_class_fields["disabled"],
}
for name in base_class_fields["disabled"]:
if name in enabled:
del disabled[name]

setattr(
cls,
_FIELDS_INFO_ATTRIBUTE_READ,
{"enabled": enabled, "disabled": disabled},
)
with suppress(AttributeError):
delattr(cls, _FIELDS_INFO_ATTRIBUTE_WRITE)

Expand All @@ -54,6 +81,7 @@ def field(
cached: bool = False,
meta: Optional[dict] = None,
out: Optional[List[Callable]] = None,
disabled: bool = False,
):
"""
Page Object method decorated with ``@field`` decorator becomes a property,
Expand Down Expand Up @@ -85,10 +113,11 @@ def __init__(self, method):

def __set_name__(self, owner, name):
if not hasattr(owner, _FIELDS_INFO_ATTRIBUTE_WRITE):
setattr(owner, _FIELDS_INFO_ATTRIBUTE_WRITE, {})
setattr(owner, _FIELDS_INFO_ATTRIBUTE_WRITE, _fields_template())

field_info = FieldInfo(name=name, meta=meta, out=out)
getattr(owner, _FIELDS_INFO_ATTRIBUTE_WRITE)[name] = field_info
field_info = FieldInfo(name=name, meta=meta, out=out, disabled=disabled)
switch = "disabled" if disabled else "enabled"
getattr(owner, _FIELDS_INFO_ATTRIBUTE_WRITE)[switch][name] = field_info

def __get__(self, instance, owner=None):
return self.unbound_method(instance)
Expand Down Expand Up @@ -125,12 +154,21 @@ def processed(*args, **kwargs):
return _field


def get_fields_dict(cls_or_instance) -> Dict[str, FieldInfo]:
def get_fields_dict(
cls_or_instance, include_disabled: bool = False
) -> Dict[str, FieldInfo]:
"""Return a dictionary with information about the fields defined
for the class: keys are field names, and values are
:class:`web_poet.fields.FieldInfo` instances.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: update this docstring when disabled has been renamed to a better one.

"""
return getattr(cls_or_instance, _FIELDS_INFO_ATTRIBUTE_READ, {})
fields_info = getattr(
cls_or_instance, _FIELDS_INFO_ATTRIBUTE_READ, _fields_template()
)
fields_dict = {}
fields_dict.update(fields_info["enabled"])
if include_disabled:
fields_dict.update(fields_info["disabled"])
return fields_dict


T = TypeVar("T")
Expand Down