upstash / redis-py Goto Github PK

This one is optional, but we should be using static analysis and linter tools to have a better codebase. A popular stack is: mypy, black, ruff, and isort. We can make use of these tools.

• The minimum supported Python version of 3.11 is too recent. Most of the ecosystem have support for Python 3.8 and even for 3.7. Given that we are very close to the end-of-life of Python 3.7, I believe we can set to minimum Python version to 3.8.

As discussed in #5

seconds -> ex
milliseconds -> px
unix_time_seconds -> exat
unix_time_milliseconds -> pxat

Here are my initial views on the current state of the library

• I believe the name of the library should be changed from upstash_py to upstash_redis. The py suffix does not add any value, and we have other products, so giving a more descriptive name with redis suffix would be better.
• The minimum supported Python version of 3.11 is too recent. Most of the ecosystem have support for Python 3.8 and even for 3.7. Given that we are very close to the end-of-life of Python 3.7, I believe we can set to minimum Python version to 3.8.
• Asyncio is not dominant yet. Providing an API only for it might hurt the adoption of the library. I think we should do what most of the other libraries do, and provide sync + async APIs on the same package, and the default implementation should have sync APIs: Basically, upstash_redis.Redis and upstash_redis.aio.Redis, which provides the sync and async versions of the same API.
• 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.
• The current test suite is not enough. There are lots of commands which we don't have even a single test for. And, in my local environment, most of the tests are failing. We should fix this, and improve the test suite.
• Current type hints have some problems. Running mypy shows 33 errors. In the strict mode, the error count is currently at 219.
• We don't have any CI/CD set. We should make sure that the tests are passing in Github Actions.
• This one is optional, but we should be using static analysis and linter tools to have a better codebase. A popular stack is: mypy, black, ruff, and isort. We can make use of these tools.
• We should re-export our public APIs in __init__.py so that what is public could be detemined easily and the public APIs could be used easier.

I believe the name of the library should be changed from upstash_py to upstash_redis. The py suffix does not add any value, and we have other products, so giving a more descriptive name with redis suffix would be better.

In sync_execute, commands are executed by making requests to the Upstash REST API like this...

response = session.post(url, headers=headers, json=command).json()

...where session is a requests.Session instance.

As indicated by the requests documentation, this call should include a timeout to avoid blocking for long periods in the event of networking trouble:

Nearly all production code should use this parameter [timeout] in nearly all requests. Failure to do so can cause your program to hang indefinitely

For the Redis workloads I typically have, no response from Redis is better than a slow response from Redis. The client should at least allow users to configure a timeout for these requests, and in my opinion should also have a default timeout enabled without user intervention.

• Asyncio is not dominant yet. Providing an API only for it might hurt the adoption of the library. I think we should do what most of the other libraries do, and provide sync + async APIs on the same package, and the default implementation should have sync APIs: Basically, upstash_redis.Redis and upstash_redis.aio.Redis, which provides the sync and async versions of the same API.

• 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.

We should re-export our public APIs in init.py so that what is public could be detemined easily and the public APIs could be used easier.

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

The current test suite is not enough. There are lots of commands which we don't have even a single test for. And, in my local environment, most of the tests are failing. We should fix this, and improve the test suite.

The typescript client supports these commands: https://upstash.com/docs/oss/sdks/ts/redis/commands/transaction. Curious to know if these will be available in the Python SDK any time soon?

Pipeline Typescript SDK Usage

const p = redis.pipeline()
p.set('foo', 'bar')
p.get('foo')
const res = await p.exec()

Transaction Typescript SDK Usage

const tx = redis.multi()
tx.set('foo', 'bar')
tx.get('foo')
const res = await tx.exec()