Skip to content
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

Event hooks #1246

Merged
merged 23 commits into from
Sep 15, 2020
Merged

Event hooks #1246

merged 23 commits into from
Sep 15, 2020

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Sep 2, 2020

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:

client = httpx.Client()
assert dict(client.event_hooks) == {'request': [], 'response': []}

client.event_hooks['request'] = [some_callable]
assert dict(client.event_hooks) == {'request': [some_callable], 'response': []}

client.event_hooks['request'].append(some_other_callable)
assert dict(client.event_hooks) == {'request': [some_callable, some_other_callable], 'response': []}

client.event_hooks['request'] = []
assert dict(client.event_hooks) == {'request': [], 'response': []}

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 would client.event_hooks['request'] = some_callable) but on the flip side it's a bit simpler without the EventHooks mapping 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...

Screenshot 2020-09-02 at 10 45 10

@tomchristie tomchristie mentioned this pull request Sep 2, 2020
httpx/_client.py Outdated Show resolved Hide resolved
docs/advanced.md Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
tests/client/test_event_hooks.py Outdated Show resolved Hide resolved
@tomchristie
Copy link
Member Author

tomchristie commented Sep 2, 2020

Here's the behaviour we'll want for event hooks...

  • request - Called exactly once, with callback(request). We want the fully prepared request at this point, including any auth headers.
  • auth - Usually not called, except for multiple auth flows, where it's called with callback(response, request).
  • redirect - Called for redirects, with callback(response, request).
  • response - Called exactly once, with callback(response).

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 redirect and auth smooshed down into a single case. (Probably still just labelled redirect) since you probably always want to handle the "multiple requests got made" in the same way.

@tomchristie
Copy link
Member Author

Prompting me to think that we've kinda got our send_handling_auth and send_handling_redirects around the wrong way, really.

@tomchristie tomchristie added the enhancement New feature or request label Sep 14, 2020
@tomchristie tomchristie changed the title Event hooks (revisited) Event hooks Sep 14, 2020
@tomchristie
Copy link
Member Author

@florimondmanca 🙏 😁

@florimondmanca
Copy link
Member

florimondmanca commented Sep 14, 2020

@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.

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good! 👍👍

@tomchristie tomchristie merged commit 54f7708 into master Sep 15, 2020
@tomchristie tomchristie deleted the event-hooks-revisited branch September 15, 2020 11:05
@tomchristie tomchristie mentioned this pull request Sep 21, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Event Hooks
2 participants