Repository: hhursev/recipe-scrapers
Pull request #996 (2024-01-29 21:15:18)

Refactor the utils.get_minutes method
The `_utils.get_minutes` method has a high McCabe cyclomatic complexity, meaning that [`flake8` would complain](https://flake8.pycqa.org/en/latest/user/options.html#cmdoption-flake8-max-complexity) about it if we didn't [suppress the relevant `C901` warning](https://github.com/hhursev/recipe-scrapers/blob/663b7500b57c6c51314c3c308c31be508749c6b2/recipe_scrapers/_utils.py#L63).

Although not perfect, a high complexity score is usually a good indicator that some code could be refactored into something cleaner.

I've wanted to try this for a while, and I'll see how far I get with this branch.  There are a few inter-dependencies (exception handling, plugins, zero-value handling, ...), not to mention that we only have a few tests per scraper, so a few experiments may be required along the way.

jayaddison: @hhursev @jknndy @strangetom I feel like I could tinker with this refactor forever, but should probably stop.  What do you think of the changes?  Any other / additional ideas for cleanup?

(the `schemaorg` constructor could be next on the refactor project list)

Patch:
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 4fdf487c..70ab59eb 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -30,3 +30,8 @@ repos:
     hooks:
     - id: mypy
       additional_dependencies: [types-beautifulsoup4, types-requests]
+-   repo: https://github.com/asottile/pyupgrade
+    rev: v3.15.0
+    hooks:
+    - id: pyupgrade
+      args: [--py38-plus]
diff --git a/pyproject.toml b/pyproject.toml
index 9503efd7..f8756f8a 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -18,6 +18,7 @@ classifiers = [
     "Programming Language :: Python :: 3.9",
     "Programming Language :: Python :: 3.10",
     "Programming Language :: Python :: 3.11",
+    "Programming Language :: Python :: 3.12",
     "License :: OSI Approved :: MIT License",
     "Intended Audience :: Developers",
     "Operating System :: OS Independent",
diff --git a/recipe_scrapers/__init__.py b/recipe_scrapers/__init__.py
index 1c87330b..fdf7b53c 100644
--- a/recipe_scrapers/__init__.py
+++ b/recipe_scrapers/__init__.py
@@ -1,7 +1,7 @@
 from __future__ import annotations
 
 import contextlib
-from typing import Any, Optional
+from typing import Any
 
 from ._abstract import AbstractScraper
 from ._exceptions import NoSchemaFoundInWildMode, WebsiteNotImplementedError
@@ -662,7 +662,7 @@ def scrape_me(url_path: str, **options: Any) -> AbstractScraper:
 
 
 def scrape_html(
-    html: str, org_url: Optional[str] = None, **options: dict[str, Any]
+    html: str, org_url: str | None = None, **options: dict[str, Any]
 ) -> AbstractScraper:
     """
     takes a string of html and returns a scraper object. if the org_url is specified
@@ -672,7 +672,7 @@ def scrape_html(
 
     Args:
         html (str): raw HTML in text form
-        org_url (Optional[str], optional): Original URL of the HTML. Defaults to None.
+        org_url (str, optional): Original URL of the HTML. Defaults to None.
 
     Raises:
         NoSchemaFoundInWildMode: If no schema is found in wild mode.
diff --git a/recipe_scrapers/_grouping_utils.py b/recipe_scrapers/_grouping_utils.py
index e67ba350..5176240c 100644
--- a/recipe_scrapers/_grouping_utils.py
+++ b/recipe_scrapers/_grouping_utils.py
@@ -45,8 +45,8 @@ def score_sentence_similarity(first: str, second: str) -> float:
         # so the score is 0
         return 0
 
-    first_bigrams = set([first[i : i + 2] for i in range(len(first) - 1)])
-    second_bigrams = set([second[i : i + 2] for i in range(len(second) - 1)])
+    first_bigrams = {first[i : i + 2] for i in range(len(first) - 1)}
+    second_bigrams = {second[i : i + 2] for i in range(len(second) - 1)}
 
     intersection = first_bigrams & second_bigrams
 
diff --git a/recipe_scrapers/_schemaorg.py b/recipe_scrapers/_schemaorg.py
index 7b6a7460..03442200 100644
--- a/recipe_scrapers/_schemaorg.py
+++ b/recipe_scrapers/_schemaorg.py
@@ -1,7 +1,7 @@
 # mypy: disallow_untyped_defs=False
 # IF things in this file continue get messy (I'd say 300+ lines) it may be time to
 # find a package that parses https://schema.org/Recipe properly (or create one ourselves).
-
+from __future__ import annotations
 
 from itertools import chain
 
@@ -126,35 +126,42 @@ class SchemaOrg:
         if author:
             return author.strip()
 
+    def _read_duration_field(self, k: str) -> int | None:
+        v = self.data.get(k)
+        if v is None:
+            return None
+        if isinstance(v, str):
+            return get_minutes(v)
+        # Workaround: strictly speaking schema.org does not provide for minValue and maxValue properties on objects of type Duration; they are however present on objects with type QuantitativeValue
+        # Refs:
+        #  - https://schema.org/Duration
+        #  - https://schema.org/QuantitativeValue
+        if isinstance(v, dict) and v.get("maxValue"):
+            return get_minutes(v["maxValue"])
+        return None
+
     def total_time(self):
         if not (self.data.keys() & {"totalTime", "prepTime", "cookTime"}):
             raise SchemaOrgException("Cooking time information not found in SchemaOrg")
 
-        def get_key_and_minutes(k):
-            source = self.data.get(k)
-            # Workaround: strictly speaking schema.org does not provide for minValue (and maxValue) properties on objects of type Duration; they are however present on objects with type QuantitativeValue
-            # Refs:
-            #  - https://schema.org/Duration
-            #  - https://schema.org/QuantitativeValue
-            if type(source) is dict and "minValue" in source:
-                source = source["minValue"]
-            return get_minutes(source, return_zero_on_not_found=True)
-
-        total_time = get_key_and_minutes("totalTime")
-        if not total_time:
-            times = list(map(get_key_and_minutes, ["prepTime", "cookTime"]))
-            total_time = sum(times)
-        return total_time
+        total_time = self._read_duration_field("totalTime")
+        if total_time:
+            return total_time
+
+        prep_time = self._read_duration_field("prepTime") or 0
+        cook_time = self._read_duration_field("cookTime") or 0
+        if prep_time or cook_time:
+            return prep_time + cook_time
 
     def cook_time(self):
         if not (self.data.keys() & {"cookTime"}):
             raise SchemaOrgException("Cooktime information not found in SchemaOrg")
-        return get_minutes(self.data.get("cookTime"), return_zero_on_not_found=True)
+        return self._read_duration_field("cookTime")
 
     def prep_time(self):
         if not (self.data.keys() & {"prepTime"}):
             raise SchemaOrgException("Preptime information not found in SchemaOrg")
-        return get_minutes(self.data.get("prepTime"), return_zero_on_not_found=True)
+        return self._read_duration_field("prepTime")
 
     def yields(self):
         if not (self.data.keys() & {"recipeYield", "yield"}):
diff --git a/recipe_scrapers/_utils.py b/recipe_scrapers/_utils.py
index 4b9a3e20..15294613 100644
--- a/recipe_scrapers/_utils.py
+++ b/recipe_scrapers/_utils.py
@@ -60,11 +60,27 @@ RECIPE_YIELD_TYPES = (
 )
 
 
-def get_minutes(element, return_zero_on_not_found=False):  # noqa: C901: TODO
+def _extract_fractional(input_string: str) -> float:
+    input_string = input_string.strip()
+    if "/" in input_string:
+        # for example "1 1/2" is matched
+        components = input_string.split(" ")
+        whole_part, fractional_part = components[0], components[-1]
+        numerator, denominator = fractional_part.split("/")
+        return float(whole_part) + float(int(numerator) / int(denominator))
+
+    whole_part, fractional_amount = "", 0.0
+    for symbol in input_string:
+        if symbol in FRACTIONS:
+            fractional_amount += FRACTIONS[symbol]
+        else:
+            whole_part += symbol
+
+    return float(whole_part) + float(fractional_amount)
+
+
+def get_minutes(element):
     if element is None:
-        # to be removed
-        if return_zero_on_not_found:
-            return 0
         raise ElementNotFoundInHtml(element)
 
     # handle integer in string literal
@@ -79,61 +95,31 @@ def get_minutes(element, return_zero_on_not_found=False):  # noqa: C901: TODO
         time_text = element.get_text()
 
     # attempt iso8601 duration parsing
-    if time_text.startswith("PT"):
+    if time_text.startswith("P") and "T" in time_text:
         try:
             duration = isodate.parse_duration(time_text)
             return math.ceil(duration.total_seconds() / 60)
         except Exception:
             pass
 
-    if time_text.startswith("P") and "T" in time_text:
-        time_text = time_text.split("T", 2)[1]
-    if "-" in time_text:
-        time_text = time_text.split("-", 2)[
-            1
-        ]  # sometimes formats are like this: '12-15 minutes'
-    if " to " in time_text:
-        time_text = time_text.split("to", 2)[
-            1
-        ]  # sometimes formats are like this: '12 to 15 minutes'
-    if "h" in time_text:
-        time_text = time_text.replace("h", "hours")
-
-    matched = TIME_REGEX.search(time_text)
-
-    if matched is None or not any(matched.groupdict().values()):
+    if "-" in time_text:  # sometimes formats are like this: '12-15 minutes'
+        _min, _, time_text = time_text.partition("-")
+    if " to " in time_text:  # sometimes formats are like this: '12 to 15 minutes'
+        _min, _to, time_text = time_text.partition(" to ")
+
+    time_units = TIME_REGEX.search(time_text).groupdict()
+    if not any(time_units.values()):
         return None
 
-    minutes = int(matched.groupdict().get("minutes") or 0)
-    hours_matched = matched.groupdict().get("hours")
-    days_matched = matched.groupdict().get("days")
+    minutes_matched = time_units.get("minutes")
+    hours_matched = time_units.get("hours")
+    days_matched = time_units.get("days")
 
     # workaround for formats like: 0D4H45M, that are not a valid iso8601 it seems
-    if days_matched:
-        minutes += 60 * 60 * float(days_matched.strip())
-    if hours_matched:
-        hours_matched = hours_matched.strip()
-        if any([symbol in FRACTIONS.keys() for symbol in hours_matched]):
-            hours = 0
-            for fraction, value in FRACTIONS.items():
-                if fraction in hours_matched:
-                    hours += value
-                    hours_matched = hours_matched.replace(fraction, "")
-            hours += int(hours_matched) if hours_matched else 0
-        elif "/" in hours_matched:
-            # for example "1 1/2" is matched
-            hours_matched_split = hours_matched.split(" ")
-            hours = 0
-            if len(hours_matched_split) == 2:
-                hours += int(hours_matched_split[0])
-            fraction = hours_matched_split[-1:][0].split("/")
-            hours += float(int(fraction[0]) / int(fraction[1]))
-        else:
-            hours = float(hours_matched)
-
-        minutes += round(60 * hours, 0)
-
-    return minutes
+    days = float(days_matched) if days_matched else 0
+    hours = float(_extract_fractional(hours_matched)) if hours_matched else 0
+    minutes = float(minutes_matched) if minutes_matched else 0
+    return minutes + round(hours * 60) + round(days * 24 * 60)
 
 
 def get_yields(element):
@@ -163,7 +149,7 @@ def get_yields(element):
 
     for singular, plural in RECIPE_YIELD_TYPES:
         if singular in serve_text_lower:
-            return "{} {}".format(matched, singular if int(matched) == 1 else plural)
+            return f"{matched} {singular if int(matched) == 1 else plural}"
 
     if SERVE_REGEX_ITEMS.search(serve_text) is not None:
         return "{} item{}".format(matched, "" if int(matched) == 1 else "s")
diff --git a/recipe_scrapers/allrecipes.py b/recipe_scrapers/allrecipes.py
index 2e7425c5..b4edb68c 100644
--- a/recipe_scrapers/allrecipes.py
+++ b/recipe_scrapers/allrecipes.py
@@ -101,12 +101,15 @@ class AllRecipesUser(AbstractScraper):
 
     def total_time(self):
         if "total" in self.meta:
-            total_time = get_minutes(self.meta["total"], return_zero_on_not_found=True)
+            return get_minutes(self.meta.get("total") or 0)
         else:
-            total_time = get_minutes(self.meta.get("cook", 0)) + get_minutes(
-                self.meta.get("prep", 0)
-            )
-        return total_time
+            return self.prep_time() + self.cook_time()
+
+    def prep_time(self):
+        return get_minutes(self.meta.get("prep") or 0)
+
+    def cook_time(self):
+        return get_minutes(self.meta.get("cook") or 0)
 
     def yields(self):
         yield_data = self.meta.get("yield")
diff --git a/recipe_scrapers/coopse.py b/recipe_scrapers/coopse.py
index 413333b7..1ebbc273 100644
--- a/recipe_scrapers/coopse.py
+++ b/recipe_scrapers/coopse.py
@@ -72,10 +72,10 @@ class CoopSE(AbstractScraper):
                     quantity = quantity[:-2]
                 unit = i["unit"]
                 if unit:
-                    unit = " {}".format(unit)
+                    unit = f" {unit}"
                 pre_preparation = i["prePreparation"]
                 if not pre_preparation == "":
-                    pre_preparation = " {}".format(pre_preparation)
+                    pre_preparation = f" {pre_preparation}"
                 name = i["name"]
                 if not quantity == "1" and not pre_preparation == " riven":
                     name = i["ingredient"]["pluralName"]
diff --git a/recipe_scrapers/fitmencook.py b/recipe_scrapers/fitmencook.py
index 11cf3c42..775ec583 100644
--- a/recipe_scrapers/fitmencook.py
+++ b/recipe_scrapers/fitmencook.py
@@ -27,7 +27,7 @@ class FitMenCook(AbstractScraper):
                     yields = word
 
         if yields:
-            return get_yields("{} servings".format(yields))
+            return get_yields(f"{yields} servings")
 
     def ingredients(self):
         ingredients_parent = self.soup.find("div", {"class": "fmc_ingredients"})
diff --git a/recipe_scrapers/nihhealthyeating.py b/recipe_scrapers/nihhealthyeating.py
index 435b3615..81f2ea7d 100644
--- a/recipe_scrapers/nihhealthyeating.py
+++ b/recipe_scrapers/nihhealthyeating.py
@@ -24,7 +24,7 @@ class NIHHealthyEating(AbstractScraper):
             raise ElementNotFoundInHtml("Table with times was not found.")
 
         return sum(
-            [get_minutes(td) for td in time_table.find_all("td") if get_minutes(td)]
+            get_minutes(td) for td in time_table.find_all("td") if get_minutes(td)
         )
 
     def yields(self):
diff --git a/recipe_scrapers/receitasnestlebr.py b/recipe_scrapers/receitasnestlebr.py
index cea7930b..57d260c6 100644
--- a/recipe_scrapers/receitasnestlebr.py
+++ b/recipe_scrapers/receitasnestlebr.py
@@ -21,7 +21,7 @@ class ReceitasNestleBR(AbstractScraper):
 
     def total_time(self):
         total_time = self.schema.total_time()
-        if total_time == 0:
+        if total_time is None:
             time_div = self.soup.find("div", {"class": "recipeDetail__infoItem--time"})
             if time_div:
                 time_str = "".join(time_div.stripped_strings)
diff --git a/recipe_scrapers/thehappyfoodie.py b/recipe_scrapers/thehappyfoodie.py
index 79ebd291..1f34e756 100644
--- a/recipe_scrapers/thehappyfoodie.py
+++ b/recipe_scrapers/thehappyfoodie.py
@@ -42,10 +42,7 @@ class TheHappyFoodie(AbstractScraper):
                 )
             )
 
-        return [
-            normalize_string("{} {}".format(amount, name))
-            for amount, name in ingredients
-        ]
+        return [normalize_string(f"{amount} {name}") for amount, name in ingredients]
 
     def instructions(self):
         return self.schema.instructions()


Test patch:
diff --git a/.github/workflows/unittests.yaml b/.github/workflows/unittests.yaml
index 36b19dcd..a996f2d1 100644
--- a/.github/workflows/unittests.yaml
+++ b/.github/workflows/unittests.yaml
@@ -14,7 +14,7 @@ jobs:
       fail-fast: false
       matrix:
         os: [ubuntu-latest, macos-13, windows-latest]
-        python-version: ["3.8", "3.9", "3.10", "3.11"]
+        python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"]
         include:
           # set toxenv to workaround-darwin on macos (check tox.ini)
           - toxenv: py
diff --git a/tests/__init__.py b/tests/__init__.py
index 08e4b812..efffd595 100644
--- a/tests/__init__.py
+++ b/tests/__init__.py
@@ -71,7 +71,7 @@ def test_func_factory(
     """
 
     def test_func(self):
-        with open(testjson, "r", encoding="utf-8") as f:
+        with open(testjson, encoding="utf-8") as f:
             expect = json.load(f)
             expect["ingredient_groups"] = [
                 IngredientGroup(**group)
diff --git a/tests/library/test_utils.py b/tests/library/test_utils.py
index 92b1a22e..a72822c1 100644
--- a/tests/library/test_utils.py
+++ b/tests/library/test_utils.py
@@ -32,6 +32,10 @@ class TestUtils(unittest.TestCase):
         text = "1.5 hours"
         self.assertEqual(90, get_minutes(text))
 
+    def test_get_minutes_integer_days(self):
+        text = "2 days"
+        self.assertEqual(2880, get_minutes(text))
+
     def test_get_minutes_fraction_with_fraction_unicode_character_halves(self):
         text = "1½ hours"
         self.assertEqual(90, get_minutes(text))
diff --git a/tests/test_data/arla.se/arla_2.json b/tests/test_data/arla.se/arla_2.json
index 3f13c088..d65fe090 100644
--- a/tests/test_data/arla.se/arla_2.json
+++ b/tests/test_data/arla.se/arla_2.json
@@ -72,6 +72,6 @@
   "ratings": 3.5,
   "site_name": "Arla",
   "title": "Trifle på äpple med smulkrisp och vaniljglass",
-  "total_time": 0,
+  "total_time": null,
   "yields": "4 servings"
 }