Introduce SelectFields

[BurnzZ]

This is related to #115 which implements approach 3.

swap_item_cls is omitted for now to reduce its complexity. We can add it later.

TODO:

• docs
• changelog

[codecov]

Codecov Report

Merging #118 (d07f361) into feat-fields-disabled (bedf6c5) will decrease coverage by 0.40%.
The diff coverage is 95.83%.

❗ Current head d07f361 differs from pull request most recent head a493052. Consider uploading reports for the commit a493052 to get more accurate results

Additional details and impacted files

@@                   Coverage Diff                    @@
##           feat-fields-disabled     #118      +/-   ##
========================================================
- Coverage                 99.57%   99.18%   -0.40%     
========================================================
  Files                        25       25              
  Lines                       937      976      +39     
========================================================
+ Hits                        933      968      +35     
- Misses                        4        8       +4     

Impacted Files	Coverage Δ
web_poet/pages.py	96.29% <93.54%> (-3.71%)	⬇️
web_poet/fields.py	99.07% <97.50%> (+0.13%)	⬆️
web_poet/__init__.py	100.00% <100.00%> (ø)
web_poet/_typing.py	90.47% <0.00%> (-9.53%)	⬇️

[BurnzZ]

I'm proposing to expose the fields_to_extract property method since it's a handy utility for frameworks like scrapy-poet to see which fields to populate.

[Gallaecio]

Should we choose a name in line with the corresponding attribute of @field to exclude or include fields from to_item output? (e.g. to_item_fields)

[BurnzZ]

The handling of unknown fields seems to be more optimal to be placed here as opposed to something like item_from_fields() which could've already extracted some fields (which could be expensive, i.e., having additional request) before erroring out.

[BurnzZ]

If an unknown field has been unintentionally passed, it'd error out with:

# omitted stack trace

File ~/dev/z/web-poet/web_poet/fields.py:235, in <dictcomp>(.0)
    233     if skip_nonitem_fields:
    234         field_names = _without_unsupported_field_names(item_cls, field_names)
--> 235 return item_cls(**{name: getattr(obj, name) for name in field_names})

AttributeError: 'SomePage' object has no attribute 'unknown_field'

I think this is descriptive enough and we don't need additional handling of unknown fields here. Handling it in the .fields_to_extract property method should be enough.

[BurnzZ]

_get_select_fields() is not being cached.

I think this should be okay since the user might want to re-use the same page instance but with different SelectFields later on:

page = SomePage(SelectFields(fields={"*": False, "x": True}))
item1 = page.to_item()

page.select_fields = SelectFields(fields={"*": False, "x": True, "y": True})
item2 = page.to_item()

Any thoughts about this usage?

[Gallaecio]

I am OK with not caching.

However, I think we should avoid covering this scenario in the docs, and I am not sure if we should even cover it in tests. Changing the value of select_fields does not feel like a very elegant API to me.

[kmike]

I think we should raise an error, not ignore wrong values.

[kmike]

It makes more sense to me to raise an error in this case.

[kmike]

Hey! Could you please elaborate on the use cases for on_unknown_field?

Also, what does unknown field mean? Does it mean that the field is not in the item, or does it mean that the field is not defined on the page object?

[BurnzZ]

does it mean that the field is not defined on the page object?

It's mostly regarding about this but because of the parity between the page object and its item class, the field should generally not be in the item as well.

The unknown field can be derived by something like set(fields_to_include) - set(fields_to_exclude). This means unknown fields that are to be excluded won't be considered as actual unknown fields.

I'm also thinking of removing this parameter and simply raise any unknown fields by default. What do you think?

[kmike]

Aha, I see.

It seems the case with field not in the item and a field not in the page object are very different.

If a field is not in the item, we should probably raise an exception. But there is a case where skip_nonitem_fields=True is used; in this case it actually could make sense to have fields in SelectFields which are not in the item.

If a field is not defined in the page object, but is present in the item, then final item.field returned by to_item is going to have a default value, None in most cases. It could also be the case that the attribute is populated in to_item method without using web-poet fields. It's not an error. It seems that we should allow to pick any fields which are present in the item, even if they're not fields of the page object.

We should have a clear answer though what happens when some of the item attributes are extracted using @fields, and some of them are added later, in the to_item method.

There is also a complicated case where there is a field defined, but then the value assigned to the final item is changed in to_item method. Probably it's fine not to support this case, but we should at least document it.

Overall the problem looks more complicated than I initially thought :)

[Gallaecio]

When I suggested such an option, I was thinking of a scenario like this one:

B team writes BPage, subclassing APage from A team, and returning BItem, which does not subclass AItem. B team might not want new fields in APage to break BPage. But they might want a warning or error to look into it eventually in case we want to do something about it.

But it can indeed be more complex than that. When a field is removed, B team may want a different resolution. For example, they might want a warning for new fields, but an exception for missing fields.

[BurnzZ]

Closing in favor of #138