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

Jump to bottom

Handle numeric values with commas in rating processor #104

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Handle numeric values with commas in rating processor #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0a255ff
Handle numeric values with commas in rating processor...
Nykakin Aug 28, 2024
e01d071
Update comment
Nykakin Aug 28, 2024
e13c6a1
Treat commas in ints and floats differently
Nykakin Aug 28, 2024
41af58c
Update test
Nykakin Aug 28, 2024
6a9b731
Add "Any" to argument hints
Nykakin Aug 28, 2024
36ca897
Silence mypy
Nykakin Aug 28, 2024
32273d8
Run linter
Nykakin Aug 28, 2024
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
24 changes: 24 additions & 0 deletions tests/test_processors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,30 @@ def gtin(self):
},
AggregateRating(ratingValue=3.8, bestRating=10.0, reviewCount=3),
),
(
{
"ratingValue": "2.12",
"bestRating": "5.0",
"reviewCount": "12",
},
AggregateRating(ratingValue=2.12, bestRating=5.0, reviewCount=12),
),
(
{
"ratingValue": "2,12",
"bestRating": "5,0",
"reviewCount": "12",
},
AggregateRating(ratingValue=2.12, bestRating=5.0, reviewCount=12),
),
(
{
"ratingValue": "2.12",
"bestRating": "5.0",
"reviewCount": "12,123",
},
AggregateRating(ratingValue=2.12, bestRating=5.0, reviewCount=12123),
),
],
)
def test_rating(input_value, expected_value):
Expand Down
30 changes: 25 additions & 5 deletions zyte_common_items/processors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from collections.abc import Iterable, Mapping
from functools import wraps
from numbers import Real
from numbers import Integral, Real
from typing import Any, Callable, List, Optional, Union

from clear_html import clean_node, cleaned_node_to_html, cleaned_node_to_text
Expand Down Expand Up @@ -31,6 +31,26 @@
)


def _to_int(value: Any) -> Any:
if isinstance(value, Integral):
return int(value)
elif isinstance(value, str):
if "," in value:
value = value.replace(",", "")
return int(value)
return value


def _to_float(value: Any) -> Any:
Copy link
Contributor

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 call it something different, like to_rating_float, to make it somewhat clear that it should not be used for values that could be greater than 999.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about moving these as local functions within rating_processor? Then it would be clear their usage is restricted to this function.

def rating_processor(value: Any, page: Any) -> Any:
    def _to_int(value: Any) -> Any:
        ...

    def _to_float(value: Any) -> Any:
        ...

    ...

That was my original idea but I've thought that perhaps we could re-use these in some other place so I left them at the top level.

Copy link
Member

Choose a reason for hiding this comment

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

I think renaming is better as we can still reuse the renamed ones if we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wRAR what naming would you propose...? Because to_rating_float sounds like something that should be used only for, well, rating.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that these functions could be useful elsewhere. I suggested to_rating_float, but something more generic, like to_small_float, would also work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't guess from name alone what a function to_small_float does.

Copy link
Contributor

@Gallaecio Gallaecio Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I’m open to other names.

But we don’t really need for the name to make it obvious what the function can do, I think the most important thing is for the name to make people think twice before they use the function for a value that could surpass 999. The details can be explained in a docstring.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, we also have https://github.com/zytedata/zyte-parsers/blob/8b161cf69df8caeb145e5244601ff93e41b287e6/zyte_parsers/aggregate_rating.py#L101 which does a similar thing for floats. I like how ints are handled in this PR though.

Copy link
Contributor

@kmike kmike Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The function in zyte-parsers also swallows the exceptions, and here it's not; it seems not silencing them is better for the case processor is processing the value returned by human-written code (though I'm not 100% sure).

if isinstance(value, Real):
return float(value)
elif isinstance(value, str):
if "," in value:
value = value.replace(",", ".")
Comment on lines +48 to +49
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: why is if necesary? Is it an optimization, and if so - does it actually help (it can go both ways - it's 2 passes over string instead of one)?

return float(value)
return value


def _get_base_url(page: Any) -> Optional[str]:
if isinstance(page, ResponseShortcutsMixin):
return page.base_url
Expand Down Expand Up @@ -292,7 +312,7 @@ def rating_processor(value: Any, page: Any) -> Any:
The input can also be a dictionary with one or more of the
:class:`~zyte_common_items.AggregateRating` fields as keys. The values for
those keys can be either final values, to be assigned to the corresponding
fields, or selector-like objects.
fields, strings to be parsed, or selector-like objects.

If a returning dictionary is missing the ``bestRating`` field and
``ratingValue`` is a selector-like object, ``bestRating`` may be extracted.
Expand Down Expand Up @@ -343,18 +363,18 @@ def aggregateRating(self):
if isinstance(review_count, (Selector, HtmlElement)):
result.reviewCount = extract_review_count(review_count)
elif review_count is not None:
result.reviewCount = int(review_count)
result.reviewCount = _to_int(review_count)

rating_value = _handle_selectorlist(value.get("ratingValue"))
if isinstance(rating_value, (Selector, HtmlElement)):
zp_rating = extract_rating(rating_value)
result.ratingValue = zp_rating.ratingValue
result.bestRating = zp_rating.bestRating
elif rating_value is not None:
result.ratingValue = float(rating_value)
result.ratingValue = _to_float(rating_value)

if (best_rating := value.get("bestRating")) is not None:
result.bestRating = float(best_rating)
result.bestRating = _to_float(best_rating)

if result.reviewCount or result.bestRating or result.ratingValue:
return result
Expand Down