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

__aexit__ should return bool | None, not None #193

Open
llucax opened this issue May 31, 2024 · 3 comments
Open

__aexit__ should return bool | None, not None #193

llucax opened this issue May 31, 2024 · 3 comments

Comments

@llucax
Copy link

llucax commented May 31, 2024

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

llucax commented Jun 3, 2024

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

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

No branches or pull requests

2 participants