Adopt sp-repo-review

[blink1073]

@Zsailer heads up, I dropped the use of inspect.signature since it doesn't mix well with mypy when from __future__ import annotations is used. Now that we're embracing typing, the type checker should be the one enforcing the signature.

[Zsailer]

The indent here was intentional, since it's part of the method from the preview code chunk.

[blink1073]

Fixed

[Zsailer]

Same here. This indent is intentional.

[blink1073]

Hmm, this was auto-formatted by blacken-docs, maybe we need to skip this file.

[blink1073]

Fixed by skipping

[Zsailer]

Thanks, @blink1073!

I dropped the use of inspect.signature since it doesn't mix well with mypy when from future import annotations is used. Now that we're embracing typing, the type checker should be the one enforcing the signature.

Thanks for pointing this out.

I added this logic to catch invalid function at runtime and raise a clear error, since 1) it's possible (and not uncommon) to define a listener/modifier function after a live instance of this class has been created and 2) it's way too easy to define an invalid function. 😅

We lose this by relying on (static) type checker's right? There is no way to validate a function signature when it's defined after the start of runtime?

[blink1073]

Actually, here's a thought. If the parameters are non-string, we use inspect.signature. If they are strings, we defer to mypy.

[Zsailer]

Looking at this again, I don't follow what the issue is.

The typing you added looks great to me. Why does the inspect.signature logic impact mypy?

[blink1073]

I couldn't use from __future__ import annotations in this file in the server PR.

Here's an illustration:

In [1]: async def listener_signature_str_ann(
   ...:             logger: "EventLogger", schema_id: "str", data: "dict"
   ...:         ) -> "None":
   ...:             """An interface for a listener."""
   ...:             ...
   ...:

In [2]: async def listener_signature(logger: object, schema_id: str, data: dict) -> None:
   ...:             """An interface for a listener."""
   ...:             ...
   ...:

In [3]: import inspect

In [4]: inspect.signature(listener_signature)
Out[4]: <Signature (logger: object, schema_id: str, data: dict) -> None>

In [5]: inspect.signature(listener_signature_str_ann)
Out[5]: <Signature (logger: 'EventLogger', schema_id: 'str', data: 'dict') -> 'None'>

In [10]: inspect.signature(listener_signature_str_ann).parameters['logger'].annotation
Out[10]: 'EventLogger'

In [11]: inspect.signature(listener_signature).parameters['logger'].annotation
Out[11]: object

[blink1073]

In #88 I added support for an annotated string, but it only works for dict, not Dict, and not dict[str, Any], etc. It is too brittle.

[Zsailer]

Oh, I see. Yeah, you're right.

Let's drop the inspect.signature. I'm convinced that the type annotation and checker is enough to help developers interact with this API. 👍

Thank you for doing this work!