Improve handling of non-string values for 'override_tag's 'default_html' argument#224
Conversation
|
fwiw, I tested the fix the warning suggests on an active project and that fixed the PL issue |
17d8815 to
2bb323a
Compare
|
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. |
bcdickinson
left a comment
There was a problem hiding this comment.
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.
93473f1 to
1eeb971
Compare
7a3f5ab to
d46d903
Compare
d46d903 to
f03ec89
Compare
b7c80be to
eaf0711
Compare
|
@bcdickinson I believe I've addressed the changes you requested - please could you re-review? |
olivierphi
left a comment
There was a problem hiding this comment.
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, 😄
Addressed in subsequent commit
…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)
Description
Fixes #211
Raises a (more) meaningful
TypeErrorif a call is made tooverride_tagwith something other than a string as thedefault_htmlarg. 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