Conversation
|
Here's the behaviour we'll want for event hooks...
That'd give us a nice symmetry where we always end up with the same number of request/response arguments across the full set of callbacks. The only possible alternative I could see to that would be to have |
|
Prompting me to think that we've kinda got our |
…lay together correctly
|
@florimondmanca 🙏 😁 |
|
@tomchristie Im generally okay with these changes and approch, assuming we're clear enough that we will want support for additional hooks than requests and responses. That said, before committing to this API I'd sure like your opinion on how this interacts with telemetry support, #1264 ? Especially the "per-request context" aspects. |
florimondmanca
left a comment
There was a problem hiding this comment.
Code looks good! 👍👍
Based on #1215 but addressing review comments from @florimondmanca.
Minor API change here is that hooks must now be set as a list of callables, and do not accept the shorthand "set a single callable" style.
Closes #790
Valid examples:
There's probably marginally more scope for accidentally breaking things than in #1215 (Eg.
del client.event_hooks['request']would end up causing a failure, as wouldclient.event_hooks['request'] = some_callable) but on the flip side it's a bit simpler without theEventHooksmapping structure.In either case I'm happy enough with this take, since if we did ever later decide to add the extra structure in #1215, then we could still choose to do so, since it would be backwards compatible with this slightly more minimal approach.
Rendered docs...