upstash / redis-py

HTTP based Python Redis Client for Serverless and Edge Functions
https://docs.upstash.com/redis
MIT License
29 stars 3 forks source link

type hints mypy error #7

Closed chronark closed 1 year ago

chronark commented 1 year ago

Current type hints have some problems. Running mypy shows 33 errors. In the strict mode, the error count is currently at 219.

cc @TudorZgimbau @burak-upstash

TudorZgimbau commented 1 year ago

mypy throws a lot of false positives because of python’s problematic generic system. The root cause for most of them is the execute function, which can return different data types and would benefit from some sort of narrowing when called.

The problem? Python’s generic function, as illustrated in the mypy docs refers to a function that takes a parameter of certain type and returns something of the same type (TypeVar), which is not the case for execute.

I’ve tried to work around that using existing python typing features, but I couldn’t. The main blocker is the lack of a generic TypeAlias, as explained in this mypy issue.

If you want to satisfy mypy for the sake of it, there are solutions. You can make a base abstract class called Command and make each function a subclass of Generic. The thing is, this will make the code unnecessary complicated and won’t block you from using the wrong types, hence cancelling the whole point of mypy.

About the strict mode, it complains a lot about not having type-parametered generic built-ins like list. Please allow me to question it’s utility since doing list[Any] silences it, which is why it was never typed in the first place.

mdumandag commented 1 year ago

If you think that mypy is producing false-positives, you can ignore those in the code with a small comment explaining why you did it. Otherwise, I think any type hinted library should pass mypy checks, and ideally in the strict mode.

TudorZgimbau commented 1 year ago

I strongly disagree on this one. While I do understand the importance of type checking and mypy helped in catching some common mistakes, I feel like we are facing some limitations of the python type system in the current implementation and adding a comment in every single place mypy is confused will satisfy it but will not add any real benefit to the developers.

I think that's one of the reasons PyCharm decided to do their own type checker, which is usually efective enough when paired with regular checks from something more strict like mypy.

I also don't really see the benefit of being fully compliant with mypy. Most of the types it gets wrong are returns from execute, which, although can be typed with generics, can't really be fully checked by any type checker since you're also responsible for specifying the return.

And adding types for the sake of satisfying mypy's strict mode absurd requirements feels like bloating the code to me.

It's your call after all, I just wanted to point out the reason or reasons I think mypy is not that helpful for this particular library.

mdumandag commented 1 year ago

The problem is, how do we know we didn't make a mistake in our type hints/code without making sure that the mypy checks no longer fail? What is the point of having type hints, if we can't make sure that they are correct, and we are in fact returning values of these particular types?

Maybe the strict mode can be a bit challenging due to its various extra checks, but I see it as a good plus. If we can have it, fantastic. If not, that is OK as well, as long as the normal checks are working.

TudorZgimbau commented 1 year ago

Tbh that's very problematic right now. I do understand what you're saying and agree with the idea of having checks in place, the only problem is how to set them up without worsening our own dx. Something that's far from ideal but worked for me during the development was using a relaxed type checker like PyCharm's to spot common mistakes like mistypes and regularly check mypy and try to filter out the false positives.

The actual helpful error - false positives mypy gave made me question it's utility as an enforcer; so much comments / workarounds would have to be made to satisfy it that I don't even know how useful it'll be anymore. And sometimes it's not even its fault, I probably should have cast-ed some types...

mdumandag commented 1 year ago

I am closing this issue now, as the library passes the non-strict checks right now.