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

Switch redis libraries #56

Merged
merged 8 commits into from
Sep 22, 2022

Conversation

euri10
Copy link
Collaborator

@euri10 euri10 commented Sep 21, 2022

Fixes #55

it was not too complicate, imho it's cool to swap an unmaintained dependency to a more "official" one, see this message in the readme https://github.com/aio-libs/aioredis-py

Aioredis is now in redis-py 4.2.0rc1+
To install, just do pip install redis>=4.2.0rc1. The code is almost the exact same. You will just need to import like so:

Copy link
Collaborator Author

@euri10 euri10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to rename the redis.py in store to redis_store to avoid namespace conflicts with the new redis library that is called also redis

if connection:
self._connection: BaseRedis = connection
elif url:
self._connection = from_url(url)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous solution always sets the connection. This change makes connection optional because it doesn't handle "else" case. Please replace "elif" with "else". We check configuration in the code above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep sorry that was bad

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well I now know why I did this to please mypy, is the assert ok for you ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, totally. Just make sure it always truthy.

@alex-oleshkevich
Copy link
Owner

The documentation may still recommend the old library.

@euri10
Copy link
Collaborator Author

euri10 commented Sep 21, 2022

The documentation may still recommend the old library.

hopefully I didnt forget something now ;)

@@ -10,13 +12,19 @@ def prefix_factory(prefix: str, key: str) -> str:
return prefix + key


if typing.TYPE_CHECKING:
BaseRedis = Redis[bytes]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it pass mypy checks if typing library is not installed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope. see below

❯ poetry remove --dev types-redis
Updating dependencies
Resolving dependencies... (0.6s)

Writing lock file

Package operations: 0 installs, 0 updates, 1 removal

  • Removing types-redis (4.3.20)
❯ mypy --config-file pyproject.toml starsessions/ tests
starsessions/stores/redis.py:4: error: Library stubs not installed for "redis.asyncio.client" (or incompatible with Python 3.9)  [import]
    from redis.asyncio.client import Redis
    ^
starsessions/stores/redis.py:4: note: Hint: "python3 -m pip install types-redis"
starsessions/stores/redis.py:4: note: (or run "mypy --install-types" to install all missing stub packages)
starsessions/stores/redis.py:4: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
starsessions/stores/redis.py:5: error: Library stubs not installed for "redis.asyncio.utils" (or incompatible with Python 3.9)  [import]
    from redis.asyncio.utils import from_url
    ^
starsessions/stores/redis.py:27: error: Variable "starsessions.stores.redis.BaseRedis" is not valid as a type  [valid-type]
            connection: typing.Optional[BaseRedis] = None,
                                        ^
starsessions/stores/redis.py:27: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
starsessions/stores/redis.py:50: error: Variable "starsessions.stores.redis.BaseRedis" is not valid as a type  [valid-type]
                self._connection: BaseRedis = connection
                                  ^
starsessions/stores/redis.py:50: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
starsessions/stores/redis.py:56: error: BaseRedis? has no attribute "get"  [attr-defined]
            value = await self._connection.get(self.prefix(session_id))
                          ^
starsessions/stores/redis.py:59: error: Returning Any from function declared to return "bytes"  [no-any-return]
            return value
            ^
starsessions/stores/redis.py:69: error: BaseRedis? has no attribute "set"  [attr-defined]
            await self._connection.set(self.prefix(session_id), data, ex=ttl)
                  ^
starsessions/stores/redis.py:73: error: BaseRedis? has no attribute "delete"  [attr-defined]
            await self._connection.delete(self.prefix(session_id))
                  ^
starsessions/stores/redis.py:76: error: BaseRedis? has no attribute "exists"  [attr-defined]
            result: int = await self._connection.exists(self.prefix(session_id))
                                ^
Found 9 errors in 1 file (checked 23 source files)
  ~/Py/starsessions on   switch_redis_library !2 ❯     

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, i meant in the client app, not in the library itself.

I am aware if someone using starsessions and redis without typings installed will have his app won't pass mypy checks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm not sure how to test that,

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well not sure I understand you, but tested in a project I'm using and I have no mypy issue without the stubs installed:

❯ poetry remove starsessions
Updating dependencies
Resolving dependencies... (5.1s)

Writing lock file

Package operations: 0 installs, 1 update, 2 removals

  • Removing aioredis (2.0.1)
  • Removing starsessions (2.0.1)
  • Updating transitions (0.9.0 3836dc4 -> 0.9.0)
❯ poetry add git+https://github.com/euri10/starsessions@switch_redis_library

Updating dependencies
Resolving dependencies... (2.9s)

Writing lock file

Package operations: 1 install, 1 update, 0 removals

  • Installing starsessions (2.0.1 834ae98)
  • Updating transitions (0.9.0 3836dc4 -> 0.9.0)
❯ make check-mypy
mypy src management tests
Success: no issues found in 45 source files

@alex-oleshkevich alex-oleshkevich merged commit 6d6606b into alex-oleshkevich:master Sep 22, 2022
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

Successfully merging this pull request may close these issues.

aioredis dependency
2 participants