search

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

Refactor public APIs and remove format_return option #31

Closed mdumandag closed 1 year ago

mdumandag commented 1 year ago

We have decided to remove the format_return option, and I have done that in this PR.

While doing that, I have noticed some deviations from the redis spec regarding the parameter names and some return values. I have updated some parameter names according to the spec.

Also, there were some inconsistencies (some methods were returing "OK" for simple string responses while others were returning boolean) regarding the return types, so I have fixed them as well.

I have also refactored some of the public APIs related to geo commands. It was cumbersome to use dict as the variadic parameter values so I converted most of them to simple tuples.

TudorZgimbau commented 1 year ago

I don't understand, why was the option removed? Also, regarding some of the namings, I wanted to avoid using reserved keywords (such as any).

mdumandag commented 1 year ago

It is mainly because it makes the type definitions longer, and forces the user to narrow the type each time the API is used to get any sensible intellisense and make their type checkers happy. For example, if a method returns a Union[bool, Literal[0, 1]], if the user is going to use the returned value, one of the things mentioned here has to be used https://mypy.readthedocs.io/en/stable/type_narrowing.html#type-narrowing-expressions

Regarding the reserved keywords, I believe it is ok to use them on parameter names

TudorZgimbau commented 1 year ago

Type narrowing was definitely a problem, but removing a non-typing feature in order to fix that seems extreme to me. There might be users who don't care about typing or don't use a strict type checker, while caring about having a non-formatted return.

mdumandag commented 1 year ago

I don't think there will be lots of complaint about it, as it is quite similar in most of other redis libraries. Besides, we can always add it later, maybe even as an extra parameter to the commands that do formatting to have a proper per-command-return-type-formatting configuration.

TudorZgimbau commented 1 year ago

Yeah, that's true. And let's document this, so people used to or reading the Redis specs don't get confused. We could also mention getting unformatted returns by building upon execute, wdyt?

mdumandag commented 1 year ago

Makes sense, but I would say lets do it later, when we write proper docs for the client. (something that we would public to read the docs, along with the API docs). I believe it is OK for now as it is generally trivial to match the type hints and redis spec (i.e. 1 or 0 can be easily mapped to bool or list of two element pairs can be easily mapped to a tuple or dict depending on the context). Besides, that is what most of the redis clients out there do, so it will be familiar to most people

TudorZgimbau commented 1 year ago

Yeah, I just wanted to point out that we should do it, the timing and schedule is up to you. And while it's true that most Redis clients I've used apply some kind of formatting to specific commands, I've also heard complaints regarding the discrepancies between the redis.io specs and the client signatures, which is especially concerning for people getting started with Redis. That's the main reason I'm advocating for this.

Regarding familiarity, besides GEO stuff, I agree the changes are mostly intuitive.