naming

[chronark]

• The implementation of the library currently deviates from the Redis command definitions on the parameter names. For example, we currently have the following API:

    async def set(
        self,
        key: str,
        value: Any,
        nx: bool = False,
        xx: bool = False,
        get: bool = False,
        seconds: int | None = None,
        milliseconds: int | None = None,
        unix_time_seconds: int | None = None,
        unix_time_milliseconds: int | None = None,
        keep_ttl: bool = False,
    ) -> str | None: ...

  However, the command definition on the Redis is as follows:

  SET key value [NX | XX] [GET] [EX seconds | PX milliseconds |
  EXAT unix-time-seconds | PXAT unix-time-milliseconds | KEEPTTL]

  IMO we should follow the command definitions. I gave an example for this specific command, but there might be other deviations.

[chronark]

what's the difference here? apart form kebab-kase and snake_case? and upper/lower case which is optional in redis?

@mdumandag

[TudorZgimbau]

what's the difference here?

The original definition uses EX, PX, etc. and we're using seconds, milliseconds, etc.

This is intentional. I want to offer our users a good DX and for accomplishing that I've changed some namings to make them easier to understand. The reasons why I don't see this as a blocker are:

• IMHO a strong percentage of the developers using the SDK also look at redis.io/commands for functionality docs. This means they won't solely rely on our function signature and might easily understand the differences.
• The order of the parameters is mostly the same as in the command definition. So copy-pasting might work.
• I have docstrings in place wherever something was changed from the original definition
• The ts SDK also makes some exceptions on this matter if I remember correctly (cc @chronark)
• The changes are meant to make the definitions more intuitive (and quite easy to switch to from an existing client), not the opposite.

[chronark]

ooh, got it let's use ex and px, naming should conform to RESP, and only adapt the casing to what is common in python

[TudorZgimbau]

Well if you want to conform to RESP you'll have to change quite a few definitions. I designed them with dx in mind, I personally wouldn't find it very intuitive to find pxat instead of unix_time_milliseconds in a codebase.

[mdumandag]

I wonder how we can draw the line for DX. For example, aren't the nx or xx parameters we currently have in the set method also quite counter-intuitive on their own?

I agree that we should simply follow the protocol definitions for the parameter names.

[TudorZgimbau]

nx, xx and ch are, if I'm not mistaking, the only exceptions I made when it comes to intuitiveness and that's because I couldn't find a better replacement (maybe even more unntuitive). It's ultimately up to you, but I highly doubt someone will have a problem simply because we named some parameters in a more intuitive way.

Bonus point, compatibility with existing clients was not a requirement at the beginning so users might have to adapt some things anyway. And we don't even know if someone really needs these compatibility guarantees.

[chronark]

Yes compatibility is not a goal as the redis API is just horrible.

Keeping the same naming is best I believe. Not only do experienced devs know what it means but newer people can Google it and don't need to jump through hoops just to understand what something means in our sdk compared to the official docs.

I'm all for making the API nicer, but renaming will cause more confusion than it's worth I believe.

[TudorZgimbau]

Makes sense. Since we're on this topic, I hope we're keeping some typing changes where they make sense. Like dict for HSET (as it's in the ts sdk) instead of a plain list and maybe even for GEOADD and ZADD.

[chronark]

yes those are good, and greatly improve the dx.

[chronark]

closing in favour of #11