Feat/allow passing answer from @on to @after hook#683
Feat/allow passing answer from @on to @after hook#683OSkrk wants to merge 4 commits intomobilityhouse:masterfrom
Conversation
astrand
left a comment
There was a problem hiding this comment.
Thanks for working on this!
| **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"
| 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.
|
|
||
|
|
||
| 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.