Handle numeric values with commas in rating processor

[Nykakin]

Ratting processor currently accept dictionaries with standard keys ratingValue, bestRating and reviewCount. It attempts to convert values using float and int calls, so that string values are handled:

>>> from zyte_common_items.processors import rating_processor
>>> 
>>> rating_processor({"bestRating": 5, "ratingValue": "3.3", "reviewCount": 12}, None)
AggregateRating(bestRating=5.0, ratingValue=3.3, reviewCount=12)

If the value of rating uses comma instead of a dot to separate decimal places, an exception is raised:

>>> rating_processor({"bestRating": 5, "ratingValue": "3,3", "reviewCount": 12}, None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/tmp/zyte_common_items/processors.py", line 354, in rating_processor
    result.ratingValue = float(rating_value)
                         ^^^^^^^^^^^^^^^^^^^
ValueError: could not convert string to float: '3,3'

Similarly, number of reviews can have comma in string value (seen in Amazon pages), e.g. 12,070 ratings. Strings such as these also raise exceptions.

Because standard schema defines that ratingValue and bestRating use Number type and revewCount uses Integer type, decided to treat strings differently depending on used type:

• If we expect real value, try replacing , with .
• If we expect integer value, try removing ,

[Gallaecio]

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.

[Nykakin]

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.

[wRAR]

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

[Nykakin]

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

[Gallaecio]

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.

[Nykakin]

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

[Gallaecio]

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.

[kmike]

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.

[kmike]

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

[kmike]

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