Improve interceptor typing

[jenstroeger]

I’ve been using this package for a while and it works great, thank you @d5h!

And because all of our code is typed I was wondering if you’d agree to tightening the type hints in this package a little bit. For example, I think

grpc-interceptor/src/grpc_interceptor/server.py

Line 21 in af11a9b

request_or_iterator: Any,

could really be

request_or_iterator: google.protobuf.message.Message | Iterable[google.protobuf.message.Message]

(assuming the gRPC implementation shuffles protobuf messages around); and for example

grpc-interceptor/src/grpc_interceptor/server.py

Line 20 in af11a9b

method: Callable,

could also be

method: Callable[
    [google.protobuf.message.Message | Iterable[google.protobuf.message.Message], grpc.ServicerContext],
    google.protobuf.message.Message
]

Not pretty, I admit, but for the sake of discussion here.

[dan-hipschman]

I don't currently have time to do this myself, but would be open to PRs. I only have a few conditions for this kind of thing:

• It doesn't require digging into private grpc or protobuf internals, which will make the types fragile and create a maintenance burden (none of your examples seem to, but other uses of Any might).
• It doesn't add any new dependencies. Dependency bloat is a pet peeve. I don't see how this would require new dependencies, so just saying.
• It doesn't require bumping the minimum version constraints of dependencies, unless they're unsupported versions (not sure stricter typing is reason enough to not support users on older versions of deps).

The tests currently run on both the minimum and current versions of dependencies, as a "good enough" way to test that the package works for the entire range of supported dependency versions. With stricter typing I might want to extend that to run mypy on both min and current versions too. I believe that would be easy to do.

If you want to take a stab at this it would be much appreciated! Thanks!

[jenstroeger]

@dan-hipschman that all makes sense, and I agree with keeping deps at a necessary minimum.

With that in mind, the above change would add a dependency on the google package to import the Message class. One could argue that that dependency likely already exists for users of the grpc-interceptor package, but it would be a new dependency for this package nonetheless.

Let me know what you prefer, or feel free to close this issue. I won’t feel personally offended or rejected 🤓

[dan-hipschman]

It looks like the google package is part of protobuf. On a bare bones Python, if I pip install protobuf then I can from google.protobuf.message import Message.

[jenstroeger]

Alright… in that case I’m happy to provide a PR for discussion, if you’d like?

[dan-hipschman]

Sure! I'm definitely pro better typing. My only concern is breaking existing users' CI checks, or missing types so valid use cases can't be type checked. The former isn't a major concern, because ideally their CI checks would only break if they had a potential bug. But it's important that the types are accurate so all valid use cases can pass type checking.

Here are a few other thoughts:

• Is there a reference for what the types are? It doesn't look like the grpc package is typed, so I'm curious how you know which types are valid.
• I wonder if it's better to put the types inline, or with stub (.pyi) files. Inline is easier to read, but stub files are easier to make optional if we happen to miss some valid types. I assume there's some poetry magic that we could include in pyproject.toml that would allow people to turn off type checking if the types were in stub files.
• Let's make sure lint checks pass with the latest version of mypy. Later versions have improved inference and / or additional checks.

That's all I can think of right now. If you still want to take this up, then that's much appreciated! :)