Closed jvllmr closed 1 month ago
I fixed the linting errors ✅
Great work.
The MR currently contains 2 changes: passing redis instance during init and typing improvements.
The feature is a great addition, as shown in these examples: your example
dic = RedisDict(redis=redis.StrictRedis.from_url("redis://127.0.0.1/0"))
And this one
redis_already_part_of_project = redis.StrictRedis()
dic = RedisDict(redis=redis_already_part_of_project)
For typing there is still an ongoing discussion about what will be the best route for this project. A Discussion on the topic here.
So let's park the typing for now and focus on passing the redis instance.
Passing the redis instance can be advantageous in some scenarios, such as: your example, testing, and using RedisDict
within a project with an existing redis instance.
We just have to be sure the "redis" is not part of redis config, as it is currently possible to pass redis config:
dic = RedisDict(host='127.0.0.1', port=6380)
So if we could rescope this MR on passing redis instance during init. We can merge and release it:)
Oh, I didn't know that there already was a PR like that. Thanks for the pointer. I wanted to inherit from MutableMapping as well, but then I decided not to because it would introduce breaking changes if you want to fully comply with it.
I'm fine if I can pass my own connection for now as I won't have to parse my redis url or something like that.
On the other hand, I want to tell you why I think inheriting from MutableMapping is important in the future. I cannot tell Greybeard stories like you, but I want to share from my own experience. As you stated in the other PR, Python's dynamic nature can harbor many risks especially with inheritance. But this is where type linters like mypy and pyright save my day. My python software in private and at work have become very much safer since using them (not even in strict mode) because they help me to stop missing things a lot. Even if the underlying code does not validate my inputs, I can get some kind of insurance by asserting my types and using linters. Inheriting from MutableMapping can also bring performance improvements besides the full compliance to dictionaries. MutableMapping doesn't consume the underlying iterable by returning a list, but returns KeyView, ValueView and ItemView, which don't consume the iterator. Especially when data becomes large it is crucial to use your iterables correct in Python to get the most out of it. One iteration more makes a big difference in web projects that handle a lot of data in a single request - or any other data processing with Python. Although generators are kind of ugly to work with in Python, most of the built-ins and standard library methods return generators where they can to enable better performance. When I audit libraries before using them, not having a py.typed
file, returning closed collections instead of generators and not really allowing typing that is as close to the truth as possible are red flags for me and more often than not I find myself not using those libraries because of that. So I think inheriting from MutableMapping would be a good addition for a 3.0 version.
Btw, I like your way of documenting in PRs. You have a lot of detail in your messages and I felt like I was following your complete thought process. Keep it up. 😄
This PR does two things:
Redis.from_url
and other methods that could be used to create a redis objectvar: dict[str, ...] = RedisDict(...)
as well, but then methods like theextend_type
methods would be "lost".I'm open to feedback. Let me hear what you think :)