From 8c3e819a5fc8f702bc4cc79dee97570058f71886 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Kucharczyk?= Date: Tue, 12 May 2026 08:36:51 +0200 Subject: [PATCH] Consistent component return type --- common/COMPONENT_IMPROVEMENTS.md | 7 ++--- common/components.py | 22 ++++++------- tests/test_components.py | 54 ++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 16 deletions(-) diff --git a/common/COMPONENT_IMPROVEMENTS.md b/common/COMPONENT_IMPROVEMENTS.md index c41465b..0ee77e6 100644 --- a/common/COMPONENT_IMPROVEMENTS.md +++ b/common/COMPONENT_IMPROVEMENTS.md @@ -15,11 +15,8 @@ - `games/templatetags/randomid.py` uses the same hash-based approach - Fixes: caching (Popover output now cacheable), page consistency, thread safety -### 1. Inconsistent return types -`Div()`/`A()`/`Button()` return `str`, but `LinkedPurchase()`/`NameWithIcon()` return `SafeText`. -Forces callers to remember `mark_safe()` wrapping. - -**Fix**: Standardize — all component functions should return the same type. +### 1. Inconsistent return types (completed) +All component functions now return `SafeText` and are annotated accordingly. Redundant `mark_safe()` wrappers removed from `LinkedPurchase()` and `NameWithIcon()`. ### 2. Fragile A() URL resolution Tries `reverse(url)` first, then falls back to literal string. Uses `type(url) is str` diff --git a/common/components.py b/common/components.py index 9f76b3b..066175a 100644 --- a/common/components.py +++ b/common/components.py @@ -40,7 +40,7 @@ def Component( children: list[HTMLTag] | HTMLTag = [], template: str = "", tag_name: str = "", -) -> HTMLTag: +) -> SafeText: if not tag_name and not template: raise ValueError("One of template or tag_name is required.") if isinstance(children, str): @@ -130,7 +130,7 @@ def A( attributes: list[HTMLAttribute] = [], children: list[HTMLTag] | HTMLTag = [], url: str | Callable[..., Any] = "", -): +) -> SafeText: """ Returns the HTML tag "a". "url" can either be: @@ -161,7 +161,7 @@ def Button( size: str = "base", icon: bool = False, color: str = "blue", -): +) -> SafeText: return Component( template="cotton/button.html", attributes=attributes @@ -178,7 +178,7 @@ def Button( def Div( attributes: list[HTMLAttribute] = [], children: list[HTMLTag] | HTMLTag = [], -): +) -> SafeText: return Component(tag_name="div", attributes=attributes, children=children) @@ -186,7 +186,7 @@ def Input( type: str = "text", attributes: list[HTMLAttribute] = [], children: list[HTMLTag] | HTMLTag = [], -): +) -> SafeText: return Component( tag_name="input", attributes=attributes + [("type", type)], children=children ) @@ -197,7 +197,7 @@ def Form( method="get", attributes: list[HTMLAttribute] = [], children: list[HTMLTag] | HTMLTag = [], -): +) -> SafeText: return Component( tag_name="form", attributes=attributes + [("action", action), ("method", method)], @@ -208,7 +208,7 @@ def Form( def Icon( name: str, attributes: list[HTMLAttribute] = [], -): +) -> SafeText: try: result = Component(template=f"cotton/icon/{name}.html", attributes=attributes) except TemplateDoesNotExist: @@ -254,7 +254,7 @@ def LinkedPurchase(purchase: Purchase) -> SafeText: ), ], ) - return mark_safe(A(url=link, children=[a_content])) + return A(url=link, children=[a_content]) def NameWithIcon( @@ -297,17 +297,17 @@ def NameWithIcon( ], ) - return mark_safe( + return ( A( url=link, children=[content], ) if create_link - else content, + else content ) -def PurchasePrice(purchase) -> str: +def PurchasePrice(purchase) -> SafeText: return Popover( popover_content=f"{floatformat(purchase.price)} {purchase.price_currency}", wrapped_content=f"{floatformat(purchase.converted_price)} {purchase.converted_currency}", diff --git a/tests/test_components.py b/tests/test_components.py index 324b72e..38ce12c 100644 --- a/tests/test_components.py +++ b/tests/test_components.py @@ -346,5 +346,59 @@ class TemplatetagRandomidTest(unittest.TestCase): self.assertTrue(all(c in "abcdef0123456789" for c in result)) +class ComponentReturnTypeTest(unittest.TestCase): + """Test that component functions return SafeText and render correctly.""" + + def setUp(self): + components.enable_cache() + components._render_cached.cache_clear() + + def tearDown(self): + components._render_cached = components._render_cached_impl + + def test_div_returns_safe_text(self): + result = components.Div([("class", "x")], "hello") + self.assertIsInstance(result, SafeText) + + def test_div_deterministic(self): + r1 = components.Div([("class", "x")], "hello") + r2 = components.Div([("class", "x")], "hello") + self.assertEqual(r1, r2) + self.assertIn('
hello
', r1) + + def test_div_no_args(self): + result = components.Div(children="test") + self.assertIsInstance(result, SafeText) + self.assertIn('
test
', result) + + def test_a_returns_safe_text(self): + result = components.A([], "link") + self.assertIsInstance(result, SafeText) + + def test_a_literal_href(self): + result = components.A([], "x", url="/literal/path") + self.assertIn('href="/literal/path"', result) + + def test_button_returns_safe_text(self): + result = components.Button([], "click") + self.assertIsInstance(result, SafeText) + self.assertIn("