refactor: apply suggested changes

questionpy_sdk/webserver/question_ui/__init__.py

@@ -91,7 +91,7 @@ def _replace_shuffled_indices(element: etree._Element, index: int, error_collect
             index_str = _int_to_roman(index).upper()
         else:
             error_collection.insert(InvalidAttributeValueError(index_element, "format", format_style))
-            _remove_element(index_element)
+            _remove_preserving_tail(index_element)
             continue
 
         # Replace the index element with the new index string
@@ -148,16 +148,14 @@ def _require_parent(node: etree._Element) -> etree._Element:
     return parent
 
 
-def _remove_preserving_tail(node: etree._Element) -> None:
-    if node.tail is not None:
-        _add_text_before(node, node.tail)
+def _remove_element(node: etree._Element) -> None:
     _require_parent(node).remove(node)
 
 
-def _remove_element(node: etree._Element) -> None:
-    parent = node.getparent()
-    if parent is not None:
-        parent.remove(node)
+def _remove_preserving_tail(node: etree._Element) -> None:
+    if node.tail is not None:
+        _add_text_before(node, node.tail)
+    _remove_element(node)
 
 
 class QuestionMetadata:
@@ -257,13 +255,15 @@ def _validate_placeholder(self, p_instruction: etree._Element) -> tuple[str, str
         """
         parsing_error = False
         if not p_instruction.text:
-            # TODO: Show an error message?
+            reference_error = PlaceholderReferenceError(
+                element=p_instruction, placeholder=None, available=self._placeholders
+            )
+            self._errors.insert(reference_error)
             return None
 
         parts = p_instruction.text.strip().split(maxsplit=1)
-        max_parts = 2
         key = parts[0]
-        clean_option = parts[1].lower() if len(parts) == max_parts else "clean"
+        clean_option = parts[1].lower() if len(parts) == 2 else "clean"  # noqa: PLR2004
         expected = ("plain", "clean", "noclean")
         if clean_option not in expected:
             option_error = InvalidCleanOptionError(element=p_instruction, option=clean_option, expected=expected)
@@ -516,7 +516,7 @@ def _validate_format_float_element(self, element: etree._Element) -> tuple[float
             # TODO: Show an error message?
             return None
 
-        # As PHP parses floats and ints differently then Python, we make the format more strict.
+        # As PHP parses floats and integers differently than Python, we enforce a stricter format.
         # E.g. parsing '20_000' or '1d1'  results in:
         # Python ->     20000       Error
         # PHP    ->     20          1

questionpy_sdk/webserver/question_ui/errors.py

@@ -15,7 +15,7 @@
 _log = logging.getLogger(__name__)
 
 
-def _comma_separated_values(values: Collection[str], opening: str, closing: str) -> str:
+def _format_human_readable_list(values: Collection[str], opening: str, closing: str) -> str:
     *values, last_value = values
     last_value = f"{opening}{last_value}{closing}"
     if not values:
@@ -54,21 +54,32 @@ def html_message(self) -> str:
 
 @dataclass(frozen=True)
 class RenderElementError(RenderError, ABC):
-    """Represents a generic element error which occurred during rendering."""
+    """A generic element error which occurred during rendering.
+
+    Attributes:
+        element: The element where the error occurred.
+        template: A template string that defines the structure of the error message.
+            It can contain placeholders corresponding to the keys in `template_kwargs`.
+            These placeholders are identified by braces ('{' and '}'), similar to `str.format`.
+            The '{element}' placeholder is predefined and resolves to a human-readable representation of `element`.
+            Providing a value with the key 'element' in `template_kwargs` will overwrite this behaviour.
+        template_kwargs: A mapping containing the values of the placeholders in `template`.
+            If a value is of type `Collection[str]`, it will be formatted in a human-readable list.
+    """
 
     element: etree._Element
     template: str
-    kwargs: Mapping[str, str | Collection[str]] = field(default_factory=dict)
+    template_kwargs: Mapping[str, str | Collection[str]] = field(default_factory=dict)
 
     def _message(self, *, as_html: bool) -> str:
         (opening, closing) = ("<code>", "</code>") if as_html else ("'", "'")
-        kwargs = {"element": f"{opening}{self.element_representation}{closing}"}
+        template_kwargs = {"element": f"{opening}{self.element_representation}{closing}"}
 
-        for key, values in self.kwargs.items():
+        for key, values in self.template_kwargs.items():
             collection = {values} if isinstance(values, str) else values
-            kwargs[key] = _comma_separated_values(collection, opening, closing)
+            template_kwargs[key] = _format_human_readable_list(collection, opening, closing)
 
-        return self.template.format(**kwargs)
+        return self.template.format_map(template_kwargs)
 
     @property
     def message(self) -> str:
@@ -105,17 +116,17 @@ def __init__(
         value: str | Collection[str],
         expected: Collection[str] | None = None,
     ):
-        kwargs = {"value": value, "attribute": attribute}
+        template_kwargs = {"value": value, "attribute": attribute}
         expected_str = ""
         if expected:
-            kwargs["expected"] = expected
+            template_kwargs["expected"] = expected
             expected_str = " Expected values are {expected}."
 
         s = "" if isinstance(value, str) or len(value) <= 1 else ""
         super().__init__(
             element=element,
             template=f"Invalid value{s} {{value}} for attribute {{attribute}} on element {{element}}.{expected_str}",
-            kwargs=kwargs,
+            template_kwargs=template_kwargs,
         )
 
 
@@ -124,32 +135,37 @@ class ConversionError(RenderElementError):
     """Could not convert a value to another type."""
 
     def __init__(self, element: etree._Element, value: str, to_type: type, attribute: str | None = None):
-        kwargs = {"value": value, "type": to_type.__name__}
+        template_kwargs = {"value": value, "type": to_type.__name__}
 
         in_attribute = ""
         if attribute:
-            kwargs["attribute"] = attribute
+            template_kwargs["attribute"] = attribute
             in_attribute = " in attribute {attribute}"
 
         template = f"Unable to convert {{value}} to {{type}}{in_attribute} at element {{element}}."
-        super().__init__(element=element, template=template, kwargs=kwargs)
+        super().__init__(element=element, template=template, template_kwargs=template_kwargs)
 
 
 @dataclass(frozen=True)
 class PlaceholderReferenceError(RenderElementError):
-    """A placeholder was referenced which was not provided."""
-
-    def __init__(self, element: etree._Element, placeholder: str, available: Collection[str]):
-        provided = "No placeholders were provided."
-        if len(available) == 1:
-            provided = "There is only one provided placeholder: {available}."
-        elif len(available) > 1:
-            provided = "These are the provided placeholders: {available}."
+    """An unknown or no placeholder was referenced."""
+
+    def __init__(self, element: etree._Element, placeholder: str | None, available: Collection[str]):
+        if placeholder is None:
+            template = "No placeholder was referenced."
+            template_kwargs = {}
+        else:
+            if len(available) == 0:
+                provided = "No placeholders were provided."
+            else:
+                provided = "These are the provided placeholders: {available}."
+            template = f"Referenced placeholder {{placeholder}} was not found. {provided}"
+            template_kwargs = {"placeholder": placeholder, "available": available}
 
         super().__init__(
             element=element,
-            template=f"Referenced placeholder {{placeholder}} was not found. {provided}",
-            kwargs={"placeholder": placeholder, "available": available},
+            template=template,
+            template_kwargs=template_kwargs,
         )
 
 
@@ -161,7 +177,7 @@ def __init__(self, element: etree._Element, option: str, expected: Collection[st
         super().__init__(
             element=element,
             template="Invalid cleaning option {option}. Available options are {expected}.",
-            kwargs={"option": option, "expected": expected},
+            template_kwargs={"option": option, "expected": expected},
         )
 
 

tests/questionpy_sdk/webserver/test_data/faulty.xhtml

@@ -1,8 +1,6 @@
 <div xmlns="http://www.w3.org/1999/xhtml" xmlns:qpy="http://questionpy.org/ns/question">
     <span qpy:feedback="unknown">Unknown feedback type.</span>
-    <span no-attribute-value>Missing attribute value.</span>
     <qpy:unknown-element/>
-    <div>Missing placeholder.<?p missing invalid-cleaning-option?></div>
     <div qpy:if-role="deviloper|scorsese">Unknown roles.</div>
     <qpy:format-float thousands-separator="maybe" precision="invalid">Unknown value.</qpy:format-float>
     <fieldset qpy:shuffle-contents="">
@@ -11,4 +9,7 @@
             <qpy:shuffled-index format="invalid"/>. A
         </label>
     </fieldset>
+    <div>Missing placeholder.<?p missing invalid-cleaning-option?></div>
+    <div>Empty placeholder.<?p      ?></div>
+    <span no-attribute-value>Missing attribute value.</span>
 </div>

tests/questionpy_sdk/webserver/test_question_ui.py

@@ -363,31 +363,33 @@ def test_should_replace_qpy_urls(renderer: QuestionUIRenderer) -> None:
 def test_errors_should_be_collected(renderer: QuestionUIRenderer) -> None:
     expected = """
         <div>
-            <span>Missing attribute value.</span>
+            <fieldset><label>Invalid shuffle format. . A</label></fieldset>
             <div>Missing placeholder.</div>
-            <fieldset><label>Invalid shuffle format.</label></fieldset>
+            <div>Empty placeholder.</div>
+            <span>Missing attribute value.</span>
         </div>
     """
     html, errors = renderer.render()
-    assert len(errors) == 10
+    assert len(errors) == 11
 
     expected_errors: list[tuple[type[RenderError], int]] = [
         # Even though the syntax error occurs after all the other errors, it should be listed first.
-        (XMLSyntaxError, 3),
+        (XMLSyntaxError, 14),
         (InvalidAttributeValueError, 2),
-        (UnknownElementError, 4),
-        (InvalidCleanOptionError, 5),
-        (PlaceholderReferenceError, 5),
-        (InvalidAttributeValueError, 6),
-        (ConversionError, 7),
-        (ConversionError, 7),
-        (InvalidAttributeValueError, 7),
-        (InvalidAttributeValueError, 11),
+        (UnknownElementError, 3),
+        (InvalidAttributeValueError, 4),
+        (ConversionError, 5),
+        (ConversionError, 5),
+        (InvalidAttributeValueError, 5),
+        (InvalidAttributeValueError, 9),
+        (InvalidCleanOptionError, 12),
+        (PlaceholderReferenceError, 12),
+        (PlaceholderReferenceError, 13),
     ]
 
     for actual_error, expected_error in zip(errors, expected_errors, strict=True):
         error_type, line = expected_error
-        assert actual_error.line == line
         assert isinstance(actual_error, error_type)
+        assert actual_error.line == line
 
     assert_html_is_equal(html, expected)