-
Notifications
You must be signed in to change notification settings - Fork 48
feat: Add an explicit module for caching #432
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
feat: Add an explicit module for caching #432
Conversation
A follow up step will support caching network info for the new device manager API, and will use the CLI to do this.
|
||
|
||
@dataclass | ||
class CacheData: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding local key would be very helpful here. That theoretically would then have everything you need for local usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is in home_data.devices.local_key
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes sorry - was conflating user data in my head - my bad
self._device_creator = device_creator | ||
self._devices: dict[str, RoborockDevice] = {} | ||
self._mqtt_session = mqtt_session | ||
|
||
async def discover_devices(self) -> list[RoborockDevice]: | ||
"""Discover all devices for the logged-in user.""" | ||
home_data = await self._home_data_api() | ||
cache_data = await self._cache.get() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a way to force an update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see if we can address this in the cache implementation e.g. caller can flush their own cache implementation if they don't want caching. If that doesn't work we can add this as an explicit flag here?
The reason why i am thinking about this is because we also may need to flush network info as well sometimes, so it seems nice to invalidate the whole cache and try to refresh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay sounds good!
Add an explicit module for caching that initially is extended to support storing network information and avoiding the need for complext Rborocksession.