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

Introduce SelectFields — alternative #138

Open
wants to merge 10 commits into
base: feat-fields-disabled
Choose a base branch
from

Conversation

BurnzZ
Copy link
Contributor

@BurnzZ BurnzZ commented Feb 21, 2023

Build on top of #120.

This is related to #115 which implements approach 3.

This is an alternative to #118 wherein:

  • SelectFields is now an optional dependency to ItemPage.
    • However, this requires the dependencies of ItemPage and WebPage to be keyword-only. EDIT: solved in fb7b7bd
  • A new item_from_select_fields() utility function has been created to fully handle Scenario A, B, and C.

TODO

Out of scope for this PR:

  • exploring on_unknown_field in SelectFields

@BurnzZ BurnzZ requested review from kmike, wRAR and Gallaecio February 21, 2023 13:52
@BurnzZ
Copy link
Contributor Author

BurnzZ commented Feb 21, 2023

The use case for this PR is ready for review, specifically the tests/test_fields.py.

tests/test_fields.py Outdated Show resolved Hide resolved
tests/test_pages.py Outdated Show resolved Hide resolved
@BurnzZ BurnzZ force-pushed the feat-select-fields-2 branch from 68d2cc2 to d09bf66 Compare February 27, 2023 11:32
@BurnzZ BurnzZ mentioned this pull request Mar 1, 2023
2 tasks
@attrs.define
class SelectFields:
"""This is used as a dependency in :class:`~.ItemPage` to control which
fields to populate its returned item class.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should rename it, and maybe even move (though not sure), because it's not necessarily about web-poet fields used to populate the item class. It's about the item attributes user wants to get in the output. Page object may not use fields at all.

Copy link
Member

Choose a reason for hiding this comment

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

Brainstorm: SelectAttributes, SelectAttrs, SelectKeys, AttributeFilter, AttrFilter, KeyFilter, ItemFilter, FilterAttributes, FilterAttrs, FilterKeys, FilterItem.

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'm inclined to have the word "item" in there for clarity. ItemFilter could mean that the page object supports different types of items and a filter is needed to choose the right one. Somehow, we may want to also include the word "attributes" for better clarity.

How about ItemAttributes?


* ``SelectFields({"name": True})`` — select one field
* ``SelectFields({"name": False})`` — unselect one field
* ``SelectFields({"*": True})`` — select all fields
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a note about disabled fields: are they selected this way or not? E.g. "select all fields, including disabled"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Speaking of disabled fields, are we good to go with this terminology? or continue considering renaming it as well as previously explored in #120 (comment).


"""

#: Fields that the page object would use to populate the
Copy link
Member

Choose a reason for hiding this comment

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

It's more about attributes of the item which user wants to get.

"""

#: Fields that the page object would use to populate the
#: :meth:`~.Returns.item_cls` it returns. It's a mapping of field names to
Copy link
Member

Choose a reason for hiding this comment

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

A mapping of item attribute names

@BurnzZ BurnzZ force-pushed the feat-fields-disabled branch from bedf6c5 to e76d42e Compare March 1, 2023 10:42
@BurnzZ BurnzZ force-pushed the feat-select-fields-2 branch from 4e210cd to 0096c8e Compare March 1, 2023 11:13
@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

Merging #138 (e316cc9) into feat-fields-disabled (e76d42e) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files
@@                   Coverage Diff                    @@
##           feat-fields-disabled     #138      +/-   ##
========================================================
+ Coverage                 98.83%   98.88%   +0.04%     
========================================================
  Files                        28       28              
  Lines                      1199     1252      +53     
========================================================
+ Hits                       1185     1238      +53     
  Misses                       14       14              
Impacted Files Coverage Δ
web_poet/__init__.py 100.00% <100.00%> (ø)
web_poet/fields.py 99.23% <100.00%> (+0.29%) ⬆️
web_poet/pages.py 100.00% <100.00%> (ø)

@@ -2,14 +2,17 @@
``web_poet.fields`` is a module with helpers for putting extraction logic
into separate Page Object methods / properties.
"""
from __future__ import annotations
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to get around the cyclic imports so we could have this annotation: page: web_poet.ItemPage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants