Skip to content

Conversation

@JordonPhillips
Copy link
Contributor

@JordonPhillips JordonPhillips commented Mar 11, 2025

Back when interceptors were first created I didn't use dataclasses because they interacted strangely with generics. Now though, that's not the case. So this updates them to be dataclasses.

Another thing I did at the time was to leave the responsibility of declaring nullability for the properties of InterceptorContext to the parametric types. So if you were at the point of the request pipeline where the request was not yet serialized, you'd have this type:

InterceptorContext[Request, None, None, None]

That would mean that accessing context.transport_request would always be typed as None. But after serialization it would be:

InterceptorContext[Request, None, TransportRequest, None]

And at that point context.transport_response will never be None.

But the drawback is that you have to do casts like this all over the request pipeline:

context_with_transport_request = cast(InterceptorContext[Input, None, HTTPRequest, None], context)

An alternative would be to have expect_transport_response that asserts it isn't null and then returns it. The drawbacks are that type system isn't telling you when it is and isn't null anymore, and there's now another function call. But now you don't have to cast the context all over the place in the request pipeline.

I'm opening this as a draft to discuss whether we should or shouldn't make the typing change (dataclass change is happening regardless).

(Note that protocol tests won't pass because the code generated request pipeline isn't updated)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant