Feat/allow passing answer from @on to @after hook #683
Conversation
| **snake_case_payload, on_response=response_payload | ||
| ) | ||
| else: | ||
| response = handler(**snake_case_payload) |
There was a problem hiding this comment.
Not so "elegant" to have 4 calls to handler(). My suggestion is to use keyword arguments instead, like I did in #264 (comment) .
There was a problem hiding this comment.
This approach was suggested by @tropxy in this comment and clarified by @OrangeTux in a later comment (both authors of this repo). There is no 4 calls to the handler, the handler is only called once depending on the parameters.
There was a problem hiding this comment.
@OSkrk what @astrand is suggesting is something like this (right @astrand ?):
if "call_unique_id" in handler_signature.parameters:
snake_case_payload["call_unique_id"] = msg.unique_id
if inject_response:
snake_case_payload["on_response"] = response_payload
response = handler(**snake_case_payload)May you try that and see if it works?
Note: I have used your current nomenclature, but if you change the code then "on_response" will be "call_response"
| @@ -356,10 +356,25 @@ async def _handle_call(self, msg): | |||
| call_unique_id_required = "call_unique_id" in handler_signature.parameters | |||
| # call_unique_id should be passed as kwarg only if is defined explicitly | |||
| # in the handler signature | |||
| inject_response = getattr(handler, "_inject_response", False) | |||
There was a problem hiding this comment.
Why check attribute on the handler, instead of checking signature parameters? This opens up for some corner cases, like if inject_response is True but after handler does not accept "on_response" kwarg. Also it is different from how call_unique_id is handled.
| @@ -56,7 +56,7 @@ def inner(*args, **kwargs): | |||
| return decorator | |||
|
|
|||
|
|
|||
| def after(action): | |||
| def after(action, inject_response=False): | |||
There was a problem hiding this comment.
can you include notes in the doc strings in respect to the new argument and its purpose?
| response = handler( | ||
| **snake_case_payload, | ||
| call_unique_id=msg.unique_id, | ||
| on_response=response_payload, |
There was a problem hiding this comment.
It would be better the term "call_response". The "on" prefix is given to functions/callbacks as an action will be performed via that callback function. Here we want to pass an argument..
| **snake_case_payload, on_response=response_payload | ||
| ) | ||
| else: | ||
| response = handler(**snake_case_payload) |
There was a problem hiding this comment.
@OSkrk what @astrand is suggesting is something like this (right @astrand ?):
if "call_unique_id" in handler_signature.parameters:
snake_case_payload["call_unique_id"] = msg.unique_id
if inject_response:
snake_case_payload["on_response"] = response_payload
response = handler(**snake_case_payload)May you try that and see if it works?
Note: I have used your current nomenclature, but if you change the code then "on_response" will be "call_response"
|
I don't have anything against this PR. I leave it up to the new maintainers to approve and merge it. I'll have a call with the new maintainers this Friday and I'll bring this PR to the table. |
This PR addresses the feature discussed here, allowing the response sent inside the @on to be passed to the @after hook for further processing (avoiding the use of temp variables). The feature enables the @after decorator to accept an
inject_responseparameter, which can be used to control whether the response from the_on_actionhandler is passed to the_after_actionhandler, and guarantees backwards compatibility.@OrangeTux , @tropxy, @HugoJP1, @mdwcrft would like to know you guy's feedback on this.