search

vmagamedov / grpclib

Pure-Python gRPC implementation for asyncio
http://grpclib.readthedocs.io
BSD 3-Clause "New" or "Revised" License
921 stars 93 forks source link

`__aexit__` should return `bool | None`, not `None` #193

Open llucax opened 1 month ago

llucax commented 1 month ago

According to Python docs:

If an exception is supplied, and the method wishes to suppress the exception (i.e., prevent it from being propagated), it should return a true value. Otherwise, the exception will be processed normally upon exit from this method.

When writing generic code, one needs to take into account that __aexit__ could return True too, so even when in the particular case of grpclib it might never return None, the return type hint should be Optional[bool], otherwise when trying to store the return value to properly forward it when writing a wrapper, mypy will complain about getting a value which can only be None for the case of grpclib.

Having a quick look at the code, I'm not even sure if grpclib isn't actually supressing some exception in some __aexit__ (for example in Stream), so there might be even a bug lurking out there. Since I'm not familiar with the code it will take me a non trivial amount of time to find out.

vmagamedov commented 1 month ago

Forwarding None return type from a wrapper with Optional[bool] return type shouldn't cause any type check errors AFAIK. Error should incur if you try to return Optional[bool] as None return type.

There is only one place where we suppress exceptions and that __aexit__ method already has Optional[bool] return type.

But method signatures, especially those which are part of public API, should be correct anyway, so I will fix them.

vmagamedov commented 1 month ago

Tried to fix signatures but then MyPy requires you to add return statements where they're not needed. So I will leave it as it is right now if there is no real issues. Feel free to update this issue with a reproducible code example which shows your issue.

llucax commented 1 month ago

I am getting typing errors, I guess it is because the type I'm calling __aexit__() to is a generic type. And I also noticed the requirement for an explicit return None, but I added it because I guess it is better to have an explicit return None if the returned value could be anything else to avoid any accidental returning with the wrong type/value.

https://github.com/frequenz-floss/frequenz-client-base-python/pull/56/files#diff-aa887f0e8b80774f9db0b36d1e975af4c39056b2438544f2aae8a89a38e920edR132-R142