Improving consistency in usage of redis-dict

[rsnk96]

Hi @Attumm

Lovely project. I am building atop it to have a slightly modified version. If you're open to it, I think it would be good to have these features integrated back upstream. Listing the points (in no particular order) below, please do let me know if you'd be open to contributions for the same:

1. Making dict-behaviour more consistent, and raising errors where it can't be done: Currently, the below code block behaves abnormally as shown below
   • Input:

   from redis_dict import RedisDict
   
   a = RedisDict()
   a["var"]={"c":1, "c2":3}
   print("Original: ", a)
   
   a["var"]["c"] = 2
   print("Updated: ", a)
   
   a.chain_set(("var","c"),2)
   print("Updated with chain set: ", a)
   

   • Output:

   Original:  {'var': {'c': 1, 'c2': 3}}
   Updated:  {'var': {'c': 1, 'c2': 3}}
   Updated with chain set:  {'var': {'c': 1, 'c2': 3}, 'var:c': 2}
   

   • Dictionaries are treated differently depending on whether they're set directly, or using the purpose-built-functions. Hence, This could result in cases where the user believes a key should have been updated, but it never did get updated.
   • There is a difference in setting dictionaries through assignment, vs adding them using .chain_set(). This is non-intuitive, and ideally the user shouldn't have to worry about this.
   • Proposed behaviour:
     1. Creation: When the user runs a["var"]={"c":1, "c2":3}, it should internally created nested keys that are managed by redis (and hence updatable) using .chain_set() internally.
     2. Updation: When the user runs a["var"]["c"] = 2, it should raise a NotImplementedError that asks users to instead use .chain_set() instead. (proposing this approach because of a limitation in python's language parser that doesn't allow it to uniquely identify if a nested setitem is being run, or a simple setitem. For more, can refer here)
2. Supporting configurable delimiters for nested keys: Currently, in the chain_xyz() set of functions, : is used as a separator. However, this isn't a very scalable choice, as : could appear in the individual keys of the hierarchy too. Hence, this should be configurable. Additionally, the default should perhaps be a non-ASCII character like '➡️'

[rsnk96]

Hi @Attumm

Do let me know if this is something of interest

[Attumm]

Hi snk96,

Currently, life is getting in the way of writing the response your message deserves.

Thanks for reaching out. Your message made my day. You found the exact point at which I left to ponder the problem but never returned to it. Let me outline the background story of the chain_ methods. We could look at it, and maybe we can even have a second chance at solving the underlying issue.

foo["a"]["b"] = 2

The question that was left open was: "Should redis-dict support nested dictionary calls?" And if the answer is yes, in which way.

redis-dict uses Redis as a key-value store. Thus, each operation will be atomic and independent, and each client can use redis-dict to connect to large servers. chain_set was a step towards solving nested calls, and it might have been better as a private method. It's also not well documented. Therefore, it seems that another group of methods is a better fit to facilitate your use case.

There is a difference in setting dictionaries through assignment vs adding them using .chain_set(). This is non-intuitive, and ideally the user shouldn't have to worry about this.

I would agree with that statement. Personally, I'm a fan of using unit tests to describe the functionality we would like to see. We could use them to come to an agreement about what "intuitive" would look like.

The configurable delimiters is great idea, and having new methods that would expand on it could work.

It would help if we had more examples. We could write them as unit tests. Example

Let's use them to outline the ideas and behaviours, of the code.

[Attumm]

Hi @rsnk96

Would you still be open/willing to discuss this issue? If not, I think it's best to close it as the next steps are unclear and would fit better within a major redis-dict release.

[rsnk96]

Hi @Attumm Sorry, I have been a bit overloaded with my work the past few weeks and haven't gotten around to checking this out. I would of course be willing to discuss the issue. Yes, I agree with TDD! However, I will not be able to propose tests for another month or so, I will get back around mid-March. I hope that works.

Side question: With valkey gaining traction, what are your thoughts on expanding redis-dict to also support valkey dbs?

[Attumm]

Hi @rsnk96,

Being overloaded with work is definitely relatable.

Great question about Valkey. Let me get back to you on that next week. :)

[Attumm]

Hi @rsnk96

Regarding Valkey support for redis-dict, given Valkey's emergence as a potential Redis replacement:

Valkey seems to work out of the box with redis-dict, which is great. The redis-dict tests are quite extensive, covering each individual Python dictionary method.

• A dedicated CI pipeline now runs the tests of redis-dict with Valkey instead of Redis.
• Added CI badge to the Readme.
• A separate valkey-dict repository and package have been created. This will allow for focused development of any Valkey specific adjustments, should they arise, without impacting the redis-dict project.

Initially, both repositories will leverage redis.py, though this will likely change in the future.

Thanks again for raising this important point.