Skip to content

Conversation

@alxbridge
Copy link
Contributor

@alxbridge alxbridge commented May 2, 2023

Description

Fixes #211

Raises a (more) meaningful TypeError if a call is made to override_tag with something other than a string as the default_html arg. This raised error provides some context for the bad call, which was not provided in the traceback when thrown directly by Django >= 4.0.

If run in Django < 4.0, this PR causes a warning to be logged instead, giving developers on those older versions advance warning of the change.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have added an appropriate CHANGELOG entry
@zerolab
Copy link
Member

zerolab commented May 3, 2023

fwiw, I tested the fix the warning suggests on an active project and that fixed the PL issue

@alxbridge alxbridge force-pushed the bug/211-default-html-strings branch 2 times, most recently from 17d8815 to 2bb323a Compare May 4, 2023 09:23
@alxbridge
Copy link
Contributor Author

NB I've pushed an updated version of this PR with an extra commit to allow the tox config to run. However, it looks like there are other PRs to address this same issue in other ways: if preferable I can remove this again, now I can see all checks have passed.

Copy link

@bcdickinson bcdickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach, but I don't think the provided type annotations are correct. It would also be great to see some tests that check the new exception behaviour.

@alxbridge alxbridge force-pushed the bug/211-default-html-strings branch from 93473f1 to 1eeb971 Compare September 14, 2023 16:32
@alxbridge alxbridge force-pushed the bug/211-default-html-strings branch 3 times, most recently from 7a3f5ab to d46d903 Compare January 10, 2024 19:33
@alxbridge alxbridge force-pushed the bug/211-default-html-strings branch from d46d903 to f03ec89 Compare January 10, 2024 19:35
@alxbridge alxbridge force-pushed the bug/211-default-html-strings branch from b7c80be to eaf0711 Compare January 10, 2024 19:45
@alxbridge
Copy link
Contributor Author

@bcdickinson I believe I've addressed the changes you requested - please could you re-review?

Copy link

@olivierphi olivierphi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a deep knowledge of the internals of this package, so my review is rather superficial...
That said, I have 2 small comments - that you're of course free to ignore as on this codebase I've never touched before you likely know much better than me what you're doing, 😄

Copy link

@olivierphi olivierphi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to approve that PR 🙂 🎈

@alxbridge alxbridge dismissed bcdickinson’s stale review January 16, 2024 11:42

Addressed in subsequent commit

@alxbridge alxbridge merged commit 1c46bc7 into torchbox:main Jan 16, 2024
CuriousLearner added a commit to lincolnloop/django-pattern-library that referenced this pull request Feb 24, 2024
…rary into jinja * 'main' of https://github.com/torchbox/django-pattern-library: Add Python 3.12 to the test matrix, drop Django 4.1 (torchbox#242) Updates for version 1.2.0 Improve handling of non-string values for 'override_tag's 'default_html' argument (torchbox#224) fix: make it work for django5.0 (updated) (torchbox#241) Upgrade GitHub Actions versions (torchbox#237) Fix typos discovered by codespell (torchbox#238)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants