🔧 Improve match_variants function (#1191)

Simplify the logic, add better docstring, and add unit tests, to make it clearer how it works.

Change the signature, to return value of `None` when no match, rather than just returning the original value.
This makes it clearer if the function actually matched anything.

Allow the source location to be passed to the function, to improve reporting warnings.

docs/directives/need.rst

@@ -63,7 +63,7 @@ Rules for specifying variant definitions
   `Sphinx-Tags <https://www.sphinx-doc.org/en/master/man/sphinx-build.html#cmdoption-sphinx-build-t>`_,
   and :ref:`needs_filter_data` as the context for filtering.
 * You can set a *need option* to multiple variant definitions by separating each definition with either
-  the ``,`` or ``;`` symbol, like ``var_a:open; ['name' in tags]:assigned``.|br|
+  the ``,`` or ``;`` symbol, like ``var_a:open; ['name' in tags]:assigned``. |br|
   With multiple variant definitions, we set the first matching variant as the *need option's* value.
 * When you set a *need option* to multiple variant definitions, you can specify the last definition as
   a default "variant-free" option which we can use if no variant definition matches. |br|

sphinx_needs/functions/functions.py

@@ -298,19 +298,23 @@ def resolve_variants_options(
             **needs_config.filter_data
         )  # Add needs_filter_data to filter context
         need_context.update(**tags)  # Add sphinx tags to filter context
+        location = (need["docname"], need["lineno"]) if need.get("docname") else None
 
         for var_option in variants_options:
-            if var_option in need and need[var_option] not in (None, "", []):
-                if not isinstance(need[var_option], (list, set, tuple)):
-                    option_value: str = need[var_option]
-                    need[var_option] = match_variants(
-                        option_value, need_context, needs_config.variants
-                    )
-                else:
-                    option_value = need[var_option]
-                    need[var_option] = match_variants(
-                        option_value, need_context, needs_config.variants
+            if (
+                var_option in need
+                and isinstance(need[var_option], (str, list, tuple, set))
+                and (
+                    result := match_variants(
+                        need[var_option],
+                        need_context,
+                        needs_config.variants,
+                        location=location,
                     )
+                )
+                is not None
+            ):
+                need[var_option] = result
 
 
 def check_and_get_content(

sphinx_needs/utils.py

@@ -6,7 +6,6 @@
 import os
 import re
 from functools import lru_cache, reduce, wraps
-from re import Pattern
 from typing import TYPE_CHECKING, Any, Callable, TypeVar
 from urllib.parse import urlparse
 
@@ -527,110 +526,85 @@ def match_string_link(
 
 
 def match_variants(
-    option_value: str | list[str],
-    keywords: dict[str, Any],
-    needs_variants: dict[str, str],
-) -> None | str | list[str]:
-    """
-    Function to handle variant option management.
-
-    :param option_value: Value assigned to an option
-    :param keywords: Data to use as filtering context
-    :param needs_variants: Needs variants data set in users conf.py
-    :return: A string, list, or None to be used as value for option.
-    :rtype: Union[str, List, None]
+    options: str | list[str] | set[str] | tuple[str, ...],
+    context: dict[str, Any],
+    variants: dict[str, str],
+    *,
+    location: str | tuple[str | None, int | None] | None = None,
+) -> str | None:
+    """Evaluate an options list and return the first matching variant.
+
+    Each item should have the format ``<expression>:<value>``,
+    where ``<expression>`` is evaluated in the context and if it is ``True``, the value is returned.
+
+    The ``<expression>`` can also be a key in the ``variants`` dict,
+    with the actual expression.
+
+    The last item in the list can be a ``<value>`` without an expression,
+    which is returned if no other variant matches.
+
+    :param options: A string (delimited by , or ;) or iterable of strings,
+        which are evaluated as variant rules
+    :param context: Mapping of variables to values used in the expressions
+    :param variants: mapping of variables to expressions
+    :param location: The source location of the option value,
+         which can be a string (the docname or docname:lineno), a tuple of (docname, lineno).
+         Used for logging warnings.
+    :return: A string if a variant is matched, else None
     """
+    if not options:
+        return None
 
-    def variant_handling(
-        variant_definitions: list[str],
-        variant_data: dict[str, Any],
-        variant_pattern: Pattern,  # type: ignore[type-arg]
-    ) -> str | None:
-        filter_context = variant_data
-        # filter_result = []
-        no_variants_in_option = False
-        variants_in_option = False
-        for variant_definition in variant_definitions:
-            # Test if definition is a variant definition
-            check_definition = variant_pattern.search(variant_definition)
-            if check_definition:
-                variants_in_option = True
-                # Separate variant definition from value to use for the option
-                filter_string, output, _ = re.split(
-                    r"(:[\w':.\-\" ]+)$", variant_definition
-                )
-                filter_string = re.sub(r"^\[|[:\]]$", "", filter_string)
-                filter_string = needs_variants.get(filter_string, filter_string)
-                try:
-                    # https://docs.python.org/3/library/functions.html?highlight=compile#compile
-                    filter_compiled = compile(filter_string, "<string>", "eval")
-                    # Set filter_context as globals and not only locals in eval()!
-                    # Otherwise, the vars not be accessed in list comprehensions.
-                    if filter_compiled:
-                        eval_result = bool(eval(filter_compiled, filter_context))
-                    else:
-                        eval_result = bool(eval(filter_string, filter_context))
-                    # First matching variant definition defines the output
-                    if eval_result:
-                        no_variants_in_option = False
-                        return output.lstrip(":")
-                except Exception as e:
-                    logger.warning(
-                        f'There was an error in the filter statement: "{filter_string}". '
-                        f"Error Msg: {e} [needs]",
-                        type="needs",
-                    )
-            else:
-                no_variants_in_option = True
-
-        if no_variants_in_option and not variants_in_option:
-            return None
-
-        # If no variant-rule is True, set to last, variant-free option. If this does not exist, set to None.
-        defaults_to = variant_definitions[-1]
-        if variants_in_option and variant_pattern.search(defaults_to):
-            return None
-        return re.sub(r"[;,] ", "", defaults_to)
-
-    split_pattern = r"([\[\]]{1}[\w=:'. \-\"]+[\[\(\{]{1}[\w=,.': \-\"]*[\]\)\}]{1}[\[\]]{1}:[\w.\- ]+)|([\[\]]{1}[\w=:.'\-\[\] \"]+[\[\]]{1}:[\w.\- ]+)|([\w.: ]+[,;]{1})"
-    variant_rule_pattern = r"^[\w'=,:.\-\"\[\] ]+:[\w'=:.\-\"\[\] ]+$"
-    variant_splitting = re.compile(split_pattern)
-    variant_rule_matching = re.compile(variant_rule_pattern)
-
-    # Handling multiple variant definitions
-    if isinstance(option_value, str):
-        multiple_variants: list[str] = variant_splitting.split(rf"""{option_value}""")
-        multiple_variants = [
+    options_list: list[str]
+    if isinstance(options, str):
+        options_list = re.split(
+            r"([\[\]]{1}[\w=:'. \-\"]+[\[\(\{]{1}[\w=,.': \-\"]*[\]\)\}]{1}[\[\]]{1}:[\w.\- ]+)|([\[\]]{1}[\w=:.'\-\[\] \"]+[\[\]]{1}:[\w.\- ]+)|([\w.: ]+[,;]{1})",
+            rf"""{options}""",
+        )
+        options_list = [
             re.sub(r"^([;, ]+)|([;, ]+$)", "", i)
-            for i in multiple_variants
+            for i in options_list
             if i not in (None, ";", "", " ")
         ]
-        if len(multiple_variants) == 1 and not variant_rule_matching.search(
-            multiple_variants[0]
-        ):
-            return option_value
-        new_option_value = variant_handling(
-            multiple_variants, keywords, variant_rule_matching
-        )
-        if new_option_value is None:
-            return option_value
-        return new_option_value
-    elif isinstance(option_value, (list, set, tuple)):
-        multiple_variants = list(option_value)
-        # In case an option value is a list (:tags: open; close), and does not contain any variant definition,
-        # then return the unmodified value
-        # options = all([bool(not variant_rule_matching.search(i)) for i in multiple_variants])
-        options = all(
-            bool(not variant_rule_matching.search(i)) for i in multiple_variants
-        )
-        if options:
-            return option_value
-        new_option_value = variant_handling(
-            multiple_variants, keywords, variant_rule_matching
-        )
-        return new_option_value
+    elif isinstance(options, (list, set, tuple)):
+        options_list = [str(opt) for opt in options]
     else:
-        return option_value
+        raise TypeError(
+            f"Option value must be a string or iterable of strings. {type(options)} found."
+        )
+
+    variant_regex = re.compile(r"^([\w'=,:.\-\"\[\] ]+):([\w'=:.\-\"\[\] ]+)$")
+    is_variant_rule = []
+    for option in options_list:
+        if not (result := variant_regex.match(option)):
+            is_variant_rule.append(False)
+            continue
+        is_variant_rule.append(True)
+        filter_string = result.group(1)
+        if filter_string.startswith("[") and filter_string.endswith("]"):
+            filter_string = filter_string[1:-1]
+        filter_string = variants.get(filter_string, filter_string)
+        try:
+            # First matching variant definition defines the output
+            if bool(eval(filter_string, context.copy())):
+                return result.group(2).lstrip(":")
+        except Exception as e:
+            logger.warning(
+                f"Error in filter {filter_string!r}: {e} [needs.variant]",
+                type="needs",
+                subtype="variant",
+                location=location,
+            )
+
+    # If there were no variant-rules, return None
+    if all(m is False for m in is_variant_rule):
+        return None
+
+    # If no variant-rule matched, set to last if it is a variant-free option
+    if is_variant_rule[-1] is False:
+        return re.sub(r"[;,] ", "", options_list[-1])
+
+    return None
 
 
 pattern = r"(https://|http://|www\.|[\w]*?)([\w\-/.]+):([\w\-/.]+)@([\w\-/.]+)"

tests/test_variants.py

@@ -1,6 +1,29 @@
 from pathlib import Path
 
 import pytest
+from sphinx.util.console import strip_colors
+
+from sphinx_needs.utils import match_variants
+
+
+@pytest.mark.parametrize(
+    "option,context,variants,expected",
+    [
+        ("", {}, {}, None),
+        ("a", {}, {}, None),
+        ("a:yes", {}, {}, None),
+        ("a:yes", {"a": False}, {}, None),
+        ("a:yes", {"a": True}, {}, "yes"),
+        ("a:yes, no", {"a": False}, {}, "no"),
+        ("a:yes; no", {"a": False}, {}, "no"),
+        ("[a and b]:yes, no", {"a": True, "b": True}, {}, "yes"),
+        ("a:yes, no", {}, {"a": "True"}, "yes"),
+        ("a:yes, no", {"b": 1}, {"a": "b == 1"}, "yes"),
+        ("a:yes, no", {"b": 2}, {"a": "b == 1"}, "no"),
+    ],
+)
+def test_match_variants(option, context, variants, expected):
+    assert match_variants(option, context, variants) == expected
 
 
 @pytest.mark.parametrize(
@@ -11,6 +34,12 @@
 def test_variant_options_html(test_app):
     app = test_app
     app.build()
+
+    warnings = strip_colors(app._warning.getvalue()).splitlines()
+    assert warnings == [
+        f"{Path(str(app.srcdir)) / 'index.rst'}:25: WARNING: Error in filter 'tag_c': name 'tag_c' is not defined [needs.variant]"
+    ]
+
     html = Path(app.outdir, "index.html").read_text()
     assert "Tags Example" in html
     assert "tags_implemented" in html