From e01d876e0457ad9953c7e6d278cb2d8a205f3216 Mon Sep 17 00:00:00 2001 From: Alex Bridge Date: Tue, 2 May 2023 15:58:52 +0100 Subject: [PATCH 1/8] Improve handling of non-string values for 'override_tag's 'default_html' parameter --- CHANGELOG.md | 5 +++++ pattern_library/monkey_utils.py | 25 ++++++++++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4655afd1..67f69d59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,11 @@ - Ensure the project root is on `sys.path` so tests etc. can be run in by Docker Compose ([#233](https://github.com/torchbox/django-pattern-library/issues/233), [#234](https://github.com/torchbox/django-pattern-library/pull/234)) - Fix URL pattern matching for template with dashes in the file name ([#229](https://github.com/torchbox/django-pattern-library/issues/229), [#230](https://github.com/torchbox/django-pattern-library/pull/230)) +### Changed + +- Disable pointer events on menu chevron to allow clicks ([#202](https://github.com/torchbox/django-pattern-library/issues/202), [#205](https://github.com/torchbox/django-pattern-library/pull/205)) +- From Django >= 4.0, calls to `Node.render()` must always return a string, but this app previously allowed non-string values to be passed in the `default_html` parameter to `override_tag`. Passing a non-string now raises a `TypeError` when using Django >= 4.0, and logs a warning for older versions ([issue #211](https://github.com/torchbox/django-pattern-library/issues/211)). + ## [1.0.1](https://github.com/torchbox/django-pattern-library/releases/tag/v1.0.1) - 2023-08-19 ### Fixed diff --git a/pattern_library/monkey_utils.py b/pattern_library/monkey_utils.py index b502b96a..11b72720 100644 --- a/pattern_library/monkey_utils.py +++ b/pattern_library/monkey_utils.py @@ -1,3 +1,4 @@ +import inspect import logging from typing import Optional @@ -11,7 +12,9 @@ def override_tag( - register: django.template.Library, name: str, default_html: Optional[str] = None + register: django.template.Library, + name: str, + default_html: Optional[type] = UNSPECIFIED, ): """ An utility that helps you override original tags for use in your pattern library. @@ -19,6 +22,9 @@ def override_tag( original_tag = register.tags[name] + # Save the caller for the override tag in case it's needed for error reporting. + trace = inspect.stack()[1] + @register.tag(name=name) def tag_func(parser, token): original_node = original_tag(parser, token) @@ -79,6 +85,23 @@ def node_render(context): # See https://github.com/torchbox/django-pattern-library/issues/166. return str(result) elif default_html is not UNSPECIFIED: + # Ensure default_html is a string. + if not isinstance(default_html, str): + if django.VERSION < (4, 0): + logger.warning( + ( + "default_html argument to override_tag should be a string to ensure compatibility " + 'with Django >= 4.0 (line %s in "%s")' + ), + trace.lineno, + trace.filename, + ) + else: + raise TypeError( + 'default_html argument to override_tag must be a string (line %s in "%s")' + % (trace.lineno, trace.filename) + ) + # Render provided default; # if no stub data supplied. return default_html From 2831543f937caf6f0c3761fc29ed646923a1a34b Mon Sep 17 00:00:00 2001 From: Alex Bridge Date: Fri, 5 May 2023 14:38:58 +0100 Subject: [PATCH 2/8] Fix type hint for default_html argument to override_tag --- pattern_library/monkey_utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pattern_library/monkey_utils.py b/pattern_library/monkey_utils.py index 11b72720..fc3ea53f 100644 --- a/pattern_library/monkey_utils.py +++ b/pattern_library/monkey_utils.py @@ -1,6 +1,6 @@ import inspect import logging -from typing import Optional +import typing import django from django.template.library import SimpleNode @@ -14,7 +14,7 @@ def override_tag( register: django.template.Library, name: str, - default_html: Optional[type] = UNSPECIFIED, + default_html: typing.Optional[typing.Any] = UNSPECIFIED, ): """ An utility that helps you override original tags for use in your pattern library. From 3dd744c137839f5f3f2ac8e9c6b204e70e8e78db Mon Sep 17 00:00:00 2001 From: Alex Bridge Date: Tue, 9 May 2023 16:32:24 +0100 Subject: [PATCH 3/8] Send deprecation note through warnings.warn() rather than the Django logger --- CHANGELOG.md | 2 +- pattern_library/monkey_utils.py | 12 +++++------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67f69d59..d389efec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +23,7 @@ ### Changed - Disable pointer events on menu chevron to allow clicks ([#202](https://github.com/torchbox/django-pattern-library/issues/202), [#205](https://github.com/torchbox/django-pattern-library/pull/205)) -- From Django >= 4.0, calls to `Node.render()` must always return a string, but this app previously allowed non-string values to be passed in the `default_html` parameter to `override_tag`. Passing a non-string now raises a `TypeError` when using Django >= 4.0, and logs a warning for older versions ([issue #211](https://github.com/torchbox/django-pattern-library/issues/211)). +- From Django >= 4.0, calls to `Node.render()` must always return a string, but this app previously allowed non-string values to be passed in the `default_html` parameter to `override_tag`. Passing a non-string now raises a `TypeError` when using Django >= 4.0, and raises a warning for older versions ([issue #211](https://github.com/torchbox/django-pattern-library/issues/211)). ## [1.0.1](https://github.com/torchbox/django-pattern-library/releases/tag/v1.0.1) - 2023-08-19 diff --git a/pattern_library/monkey_utils.py b/pattern_library/monkey_utils.py index fc3ea53f..f0ec001d 100644 --- a/pattern_library/monkey_utils.py +++ b/pattern_library/monkey_utils.py @@ -1,6 +1,7 @@ import inspect import logging import typing +import warnings import django from django.template.library import SimpleNode @@ -88,13 +89,10 @@ def node_render(context): # Ensure default_html is a string. if not isinstance(default_html, str): if django.VERSION < (4, 0): - logger.warning( - ( - "default_html argument to override_tag should be a string to ensure compatibility " - 'with Django >= 4.0 (line %s in "%s")' - ), - trace.lineno, - trace.filename, + warnings.warn( + "default_html argument to override_tag should be a string to ensure compatibility " + 'with Django >= 4.0 (line %s in "%s")' % (trace.lineno, trace.filename), + Warning, ) else: raise TypeError( From 141ca98b26e6e1134b0c84864138a093e4c86287 Mon Sep 17 00:00:00 2001 From: Alex Bridge Date: Tue, 9 May 2023 16:49:47 +0100 Subject: [PATCH 4/8] Add tests for handling non-strings in 'override_tag's 'default_html' parameter --- .../invalid_tags_test_atom.html | 5 ++++ .../invalid_tags_test_atom.yaml | 12 ++++++++ tests/templatetags/test_tags_invalid.py | 15 ++++++++++ tests/tests/test_commands.py | 1 + tests/tests/test_tags.py | 30 +++++++++++++++++++ 5 files changed, 63 insertions(+) create mode 100644 tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.html create mode 100644 tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.yaml create mode 100644 tests/templatetags/test_tags_invalid.py diff --git a/tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.html b/tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.html new file mode 100644 index 00000000..b88ab0a1 --- /dev/null +++ b/tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.html @@ -0,0 +1,5 @@ +{% load test_tags_invalid %} + +MARMA{% default_html_tag_invalid empty_string %}LADE01 +MARMA{% default_html_tag_invalid none %}LADE02 +MARMA{% default_html_tag_invalid dict %}LADE03 diff --git a/tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.yaml b/tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.yaml new file mode 100644 index 00000000..8797d477 --- /dev/null +++ b/tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.yaml @@ -0,0 +1,12 @@ +context: + test_invalid_tags: False +tags: + default_html_tag_invalid: + empty_string: + raw: '' + none: + raw: None + dict: + zero: 0 + two: 2 + five: 5 diff --git a/tests/templatetags/test_tags_invalid.py b/tests/templatetags/test_tags_invalid.py new file mode 100644 index 00000000..58426981 --- /dev/null +++ b/tests/templatetags/test_tags_invalid.py @@ -0,0 +1,15 @@ +from django import template + +from pattern_library.monkey_utils import override_tag + +register = template.Library() + + +@register.simple_tag() +def default_html_tag_invalid(arg=None): + "Just pass, never do anything" + pass + + +# Test overriding tag with a default_html that's not valid in Django >= 4.0 +override_tag(register, "default_html_tag_invalid", default_html=[1, 2, 3]) diff --git a/tests/tests/test_commands.py b/tests/tests/test_commands.py index d9da46e8..8f320f1d 100644 --- a/tests/tests/test_commands.py +++ b/tests/tests/test_commands.py @@ -16,6 +16,7 @@ def test_displays_patterns(self): call_command("render_patterns", dry_run=True, stdout=stdout, stderr=stderr) self.assertIn( """patterns/atoms/tags_test_atom/tags_test_atom.html +patterns/atoms/tags_test_atom/invalid_tags_test_atom.html patterns/atoms/test_atom/test_atom.html """, stderr.getvalue(), diff --git a/tests/tests/test_tags.py b/tests/tests/test_tags.py index ee88b900..e4f66185 100644 --- a/tests/tests/test_tags.py +++ b/tests/tests/test_tags.py @@ -1,4 +1,5 @@ from django.test import SimpleTestCase +from unittest.mock import patch from .utils import reverse @@ -43,3 +44,32 @@ def test_falsey_default_html_overide(self): self.assertContains(response, "POTATO1") self.assertContains(response, "POTANoneTO2") self.assertContains(response, "POTA0TO3") + + def test_bad_default_html_warning(self): + with patch("django.VERSION", (3, 2, 0, "final", 0)): + with self.assertWarns(Warning) as cm: + response = self.client.get( + reverse( + "pattern_library:render_pattern", + kwargs={ + "pattern_template_name": "patterns/atoms/tags_test_atom/invalid_tags_test_atom.html", + }, + ), + ) + self.assertContains(response, "MARMALADE01") + self.assertContains(response, "MARMANoneLADE02") + self.assertIn( + "default_html argument to override_tag should be a string to ensure compatibility with Django", + str(cm.warnings[0]), + ) + + def test_bad_default_html_error(self): + with patch("django.VERSION", (4, 2, 0, "final", 0)): + with self.assertRaises(TypeError) as cm: + self.client.get( + reverse( + "pattern_library:render_pattern", + kwargs={"pattern_template_name": "patterns/atoms/tags_test_atom/invalid_tags_test_atom.html"}, + ), + ) + self.assertIn("default_html argument to override_tag must be a string", str(cm.exception)) From f03ec8915634d8f37034719714dfeda0de1cf6b2 Mon Sep 17 00:00:00 2001 From: Alex Bridge Date: Wed, 10 Jan 2024 19:19:57 +0000 Subject: [PATCH 5/8] Fix tests for handling invalid default_html argument to override_tag --- pattern_library/monkey_utils.py | 3 +- ....html => invalid_tags_test_atom.html.fail} | 0 .../invalid_tags_test_atom.yaml | 12 ---- tests/tests/test_commands.py | 1 - tests/tests/test_tags.py | 66 +++++++++++++------ 5 files changed, 48 insertions(+), 34 deletions(-) rename tests/templates/patterns/atoms/tags_test_atom/{invalid_tags_test_atom.html => invalid_tags_test_atom.html.fail} (100%) delete mode 100644 tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.yaml diff --git a/pattern_library/monkey_utils.py b/pattern_library/monkey_utils.py index f0ec001d..991407f0 100644 --- a/pattern_library/monkey_utils.py +++ b/pattern_library/monkey_utils.py @@ -91,7 +91,8 @@ def node_render(context): if django.VERSION < (4, 0): warnings.warn( "default_html argument to override_tag should be a string to ensure compatibility " - 'with Django >= 4.0 (line %s in "%s")' % (trace.lineno, trace.filename), + 'with Django >= 4.0 (line %s in "%s")' + % (trace.lineno, trace.filename), Warning, ) else: diff --git a/tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.html b/tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.html.fail similarity index 100% rename from tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.html rename to tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.html.fail diff --git a/tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.yaml b/tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.yaml deleted file mode 100644 index 8797d477..00000000 --- a/tests/templates/patterns/atoms/tags_test_atom/invalid_tags_test_atom.yaml +++ /dev/null @@ -1,12 +0,0 @@ -context: - test_invalid_tags: False -tags: - default_html_tag_invalid: - empty_string: - raw: '' - none: - raw: None - dict: - zero: 0 - two: 2 - five: 5 diff --git a/tests/tests/test_commands.py b/tests/tests/test_commands.py index 8f320f1d..d9da46e8 100644 --- a/tests/tests/test_commands.py +++ b/tests/tests/test_commands.py @@ -16,7 +16,6 @@ def test_displays_patterns(self): call_command("render_patterns", dry_run=True, stdout=stdout, stderr=stderr) self.assertIn( """patterns/atoms/tags_test_atom/tags_test_atom.html -patterns/atoms/tags_test_atom/invalid_tags_test_atom.html patterns/atoms/test_atom/test_atom.html """, stderr.getvalue(), diff --git a/tests/tests/test_tags.py b/tests/tests/test_tags.py index e4f66185..6d34f0a1 100644 --- a/tests/tests/test_tags.py +++ b/tests/tests/test_tags.py @@ -1,6 +1,10 @@ -from django.test import SimpleTestCase from unittest.mock import patch +from django.shortcuts import render +from django.test import RequestFactory, SimpleTestCase + +from pattern_library import get_pattern_context_var_name + from .utils import reverse @@ -45,31 +49,53 @@ def test_falsey_default_html_overide(self): self.assertContains(response, "POTANoneTO2") self.assertContains(response, "POTA0TO3") + +class TagsTestFailCase(SimpleTestCase): def test_bad_default_html_warning(self): + """ + Test that the library raises a warning when passing a non-string `default_html` argument to `override_tag` + in Django < 4.0 + """ with patch("django.VERSION", (3, 2, 0, "final", 0)): with self.assertWarns(Warning) as cm: - response = self.client.get( - reverse( - "pattern_library:render_pattern", - kwargs={ - "pattern_template_name": "patterns/atoms/tags_test_atom/invalid_tags_test_atom.html", - }, - ), - ) - self.assertContains(response, "MARMALADE01") - self.assertContains(response, "MARMANoneLADE02") - self.assertIn( - "default_html argument to override_tag should be a string to ensure compatibility with Django", - str(cm.warnings[0]), + template_name = ( + "patterns/atoms/tags_test_atom/invalid_tags_test_atom.html.fail" ) + request = RequestFactory().get("/") + + # Rendering the template with a non-string `default_html` argument will cause Django >= 4 to raise + # a `TypeError`, which we need to catch and ignore in order to check that the warning is raised + try: + render( + request, + template_name, + context={get_pattern_context_var_name(): True}, + ) + except TypeError: + pass + + self.assertIn( + "default_html argument to override_tag should be a string to ensure compatibility with Django", + str(cm.warnings[0]), + ) def test_bad_default_html_error(self): + """ + Test that the library raises a TypeError when passing a non-string `default_html` argument to `override_tag` + in Django >= 4.0 + """ with patch("django.VERSION", (4, 2, 0, "final", 0)): with self.assertRaises(TypeError) as cm: - self.client.get( - reverse( - "pattern_library:render_pattern", - kwargs={"pattern_template_name": "patterns/atoms/tags_test_atom/invalid_tags_test_atom.html"}, - ), + template_name = ( + "patterns/atoms/tags_test_atom/invalid_tags_test_atom.html.fail" ) - self.assertIn("default_html argument to override_tag must be a string", str(cm.exception)) + request = RequestFactory().get("/") + render( + request, + template_name, + context={get_pattern_context_var_name(): True}, + ) + self.assertIn( + "default_html argument to override_tag must be a string", + str(cm.exception), + ) From eaf07112d551b5a0bde0407613517489e0757147 Mon Sep 17 00:00:00 2001 From: Alex Bridge Date: Wed, 10 Jan 2024 19:44:25 +0000 Subject: [PATCH 6/8] Update changelog --- CHANGELOG.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d389efec..3dae8434 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Changed + +- From Django >= 4.0, calls to `Node.render()` must always return a string, but this app previously allowed non-string values to be passed in the `default_html` parameter to `override_tag`. Passing a non-string now raises a `TypeError` when using Django >= 4.0, and raises a warning for older versions ([issue #211](https://github.com/torchbox/django-pattern-library/issues/211)). + ## [1.1.0](https://github.com/torchbox/django-pattern-library/releases/tag/v1.1.0) - 2023-10-25 ### Added @@ -20,11 +26,6 @@ - Ensure the project root is on `sys.path` so tests etc. can be run in by Docker Compose ([#233](https://github.com/torchbox/django-pattern-library/issues/233), [#234](https://github.com/torchbox/django-pattern-library/pull/234)) - Fix URL pattern matching for template with dashes in the file name ([#229](https://github.com/torchbox/django-pattern-library/issues/229), [#230](https://github.com/torchbox/django-pattern-library/pull/230)) -### Changed - -- Disable pointer events on menu chevron to allow clicks ([#202](https://github.com/torchbox/django-pattern-library/issues/202), [#205](https://github.com/torchbox/django-pattern-library/pull/205)) -- From Django >= 4.0, calls to `Node.render()` must always return a string, but this app previously allowed non-string values to be passed in the `default_html` parameter to `override_tag`. Passing a non-string now raises a `TypeError` when using Django >= 4.0, and raises a warning for older versions ([issue #211](https://github.com/torchbox/django-pattern-library/issues/211)). - ## [1.0.1](https://github.com/torchbox/django-pattern-library/releases/tag/v1.0.1) - 2023-08-19 ### Fixed From 82d5e4021cd296951d2b3a402e03de27b4d0c5f1 Mon Sep 17 00:00:00 2001 From: Alex Bridge Date: Mon, 15 Jan 2024 15:51:23 +0000 Subject: [PATCH 7/8] Optimise call to stack() --- pattern_library/monkey_utils.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pattern_library/monkey_utils.py b/pattern_library/monkey_utils.py index 991407f0..12eeb3e8 100644 --- a/pattern_library/monkey_utils.py +++ b/pattern_library/monkey_utils.py @@ -23,9 +23,6 @@ def override_tag( original_tag = register.tags[name] - # Save the caller for the override tag in case it's needed for error reporting. - trace = inspect.stack()[1] - @register.tag(name=name) def tag_func(parser, token): original_node = original_tag(parser, token) @@ -88,6 +85,8 @@ def node_render(context): elif default_html is not UNSPECIFIED: # Ensure default_html is a string. if not isinstance(default_html, str): + # Save the caller for the override tag in case it's needed for error reporting. + trace = inspect.stack()[1] if django.VERSION < (4, 0): warnings.warn( "default_html argument to override_tag should be a string to ensure compatibility " From b28cc294a06267300e0f476bad23c4757d871627 Mon Sep 17 00:00:00 2001 From: Alex Bridge Date: Mon, 15 Jan 2024 15:52:33 +0000 Subject: [PATCH 8/8] Make behaviour of default_html_tag_invalid() consistent with existing tests --- tests/templatetags/test_tags_invalid.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/templatetags/test_tags_invalid.py b/tests/templatetags/test_tags_invalid.py index 58426981..9d6ba7cd 100644 --- a/tests/templatetags/test_tags_invalid.py +++ b/tests/templatetags/test_tags_invalid.py @@ -7,8 +7,8 @@ @register.simple_tag() def default_html_tag_invalid(arg=None): - "Just pass, never do anything" - pass + "Just raise an exception, never do anything" + raise Exception("default_tag raised an exception") # Test overriding tag with a default_html that's not valid in Django >= 4.0