feat: Make CacheData serializable #613
Conversation
Update the CacheData class with serialization methods so that clients can use it for persistence. This provides an API for Home Assistant to use for serialization so it doesn't need to make its own.
There was a problem hiding this comment.
Pull request overview
This PR adds serialization capabilities to the CacheData class by making it inherit from RoborockBase, enabling clients like Home Assistant to serialize and deserialize cache data for persistence. The implementation allows flexibility in choosing serialization formats (pickle or JSON) through customizable serialize_fn and deserialize_fn parameters.
- Made
CacheDatainherit fromRoborockBaseto gainas_dict()andfrom_dict()methods - Extended
FileCacheto accept customizable serialization/deserialization functions - Added comprehensive parametrized tests covering both pickle and JSON serialization with various cache data scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| roborock/devices/cache.py | Made CacheData inherit from RoborockBase to enable serialization via as_dict() and from_dict() methods |
| roborock/devices/file_cache.py | Extended FileCache constructor to accept custom serialize_fn and deserialize_fn parameters, updated store_value and load_value functions to use these customizable serializers (contains debug logging statements that should be removed) |
| tests/devices/test_file_cache.py | Added parametrized tests covering empty, populated, and multi-field cache scenarios with default (pickle), explicit pickle, and JSON serialization methods |
| tests/devices/snapshots/test_file_cache.ambr | Auto-generated snapshot file containing expected test outputs for all parametrized test combinations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| serialize_fn: Callable[[Any], bytes] = pickle.dumps, | ||
| deserialize_fn: Callable[[bytes], Any] = pickle.loads, |
There was a problem hiding this comment.
does it make sense to set the dict serialization on this side rather than on the HA side? as a separate premade serialization function?
There was a problem hiding this comment.
Home Assistant is really not using FileCache at all, and has its own json store. I mostly used this for testing of the approach.
So the real value in this CL is making this library handle the "to_dict" / "from_dict" and these are just tests making sure that can be used for various serialization strategies.
There was a problem hiding this comment.
We could make file cache use json by default instead of pickle? that is not a bad idea -- we could use it in the CLI or the example that way too. But just saying Home Assistant won't use it as is.
There was a problem hiding this comment.
i'm just thinking that if we have a custom serialization function and deserialization function on the HA side that may get annoying. i.e. if we update something on CacheData, we then have to do a version bump on the HA side + change the serialization functions, rather than having the serialization happen on the library side and HA just utilizing them for it's cache
There was a problem hiding this comment.
The function ha will call is CacheData.as_dict and .from_dict. it just does it's own json loads etc.
There was a problem hiding this comment.
I don't think we can get a large change to this API done by release time.
There was a problem hiding this comment.
I don't think we can get a large change to this API done by release time.
There was a problem hiding this comment.
I don't think we can get a large change to this API done by release time.
2 participants
Update the CacheData class with serialization methods so that clients can use it for persistence. This provides an API for Home Assistant to use for serialization so it doesn't need to make its own.
Fixing home-assistant/core#157663