-
-
Notifications
You must be signed in to change notification settings - Fork 141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add custom encoder support to trigger_client_event() #349
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great little feature, let's have it.
Thank you for updating the docs, which so many contributors miss. It would be helpful in future if you also added a changelog note. Other than that, I've made some small comments which I'm working on.
src/django_htmx/http.py
Outdated
@@ -129,6 +129,7 @@ def trigger_client_event( | |||
params: dict[str, Any] | None = None, | |||
*, | |||
after: EventAfterType = "receive", | |||
encoder: type[DjangoJSONEncoder] = DjangoJSONEncoder, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's widen the type limit to json.JSONEncoder
to give more flexibility
docs/http.rst
Outdated
|
||
``after`` selects which of the ``HX-Trigger`` headers to modify: | ||
|
||
* ``"receive"``, the default, maps to ``HX-Trigger`` | ||
* ``"settle"`` maps to ``HX-Trigger-After-Settle`` | ||
* ``"swap"`` maps to ``HX-Trigger-After-Swap`` | ||
|
||
``encoder`` specifies the JSON encoder to be used in generating the JSON. By default uses |DjangoJSONEncoder|__ for its extended data type support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One sentence per line
tests/test_http.py
Outdated
class BeanCounter: | ||
def __init__(self, magic_bean_count: int): | ||
self.magic_bean_count = magic_bean_count * 2 | ||
|
||
class MyEncoder(DjangoJSONEncoder): | ||
def default(self, o): | ||
if isinstance(o, BeanCounter): | ||
return {"magic_beans": o.magic_bean_count} | ||
|
||
return super().default(o) | ||
|
||
response = HttpResponse() | ||
uuid_value = UUID("{12345678-1234-5678-1234-567812345678}") | ||
|
||
trigger_client_event( | ||
response, | ||
"showMessage", | ||
{"uuid": uuid_value, "beans": BeanCounter(1)}, | ||
encoder=MyEncoder, | ||
) | ||
|
||
assert response["HX-Trigger"] == ( | ||
'{"showMessage": {"uuid": "12345678-1234-5678-1234-567812345678", ' | ||
'"magic_beans": 2}}' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test can be shortened a bit, we don't need a UUID or the bean count to prove the encoder is used
I haven't deciphered how to set up a dev env for this project or run its tests locally, but I think this is implemented appropriately. Hoping the GH actions covers it and the test I wrote passes! Let me know if this is a desirable change (it is for my use case!) and if there's anything I can do to increase polish and/or test coverage.