-
Notifications
You must be signed in to change notification settings - Fork 51
feat: add dynamic changing status #511
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
base: main
Are you sure you want to change the base?
Conversation
🤖 Hi @Lash-L, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
_available_mop_routes = get_clean_routes(features, region) | ||
_available_mop_routes_mapping = {route.code: route.name for route in _available_mop_routes} | ||
|
||
class DeviceStatus(Status): |
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 this needs to be a protocol or something? Was hoping you may have some ideas on how to improve it
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 can see why this is set up this way.
Does it make sense for DeviceStatus to be a top level object with a DeviceFeatures + region as input to constructor?
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.
So the issue is right now, every instance of status that we do we use from_dict()
So we'd need to add it to from_dict() and constructor. Theoretically fine, but potentially bloats from_dict() calls for status so i wasn't sure.
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.
This seems similar to the traits that need to parse room names. We basically have a hacky dance to make this work:
- RoomsTrait is constructed with home data passed to constructor and no values initially
- On update, it cals _parse_data which parses the response object and merges with the home data
- Parent class uses setattr to update the object
Perhaps this could just be added to StatusTrait
?
def __init__(self, product_info: HomeDataProduct) -> None: | ||
"""Initialize the StatusTrait.""" | ||
self._product_info = product_info | ||
self._status_type = get_custom_status(self.device_info.device_features, self.device_info.region) |
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.
This is where it gets weird - To determine the status type, we need the device features, which we get from init status, which we need to be connected for.
Hoping you may have some ideas here
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.
Can device features be provided similar to how HomeDataProduct or network info are supplied? Do it up front, and we provide an interface for caching it.
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.
That's what i thought about doing but I don't know if it is possible. The Trait objects are created synchronously before we are subscribed and we could send a message to get the init status.
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.
Maybe making a separate trait for now can work, then once its loaded it can be used? This could override refresh
to refresh it first before status is loaded the first time.
Separately: Is making it part of Status
required?
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.
Separately: Is making it part of Status required?
Not explicitly, no, but i figured it would make sense. but open to doing it differently.
I could make a device features trait that gets the init status/whatever else it needs and other things rely on it? As we will need it to determine what is supported when we start doing more complicated traits? Thoughts on that?
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.
Seems worth a try? That way we can get it working then next figure out how to best use it?
this thing of combining traits together with dependencies is kinda weird but kind of simple too but not sure if it will bite us later, but may be an easy way to string things together.
This is still a WIP. But there are a few things that I need to figure out and I thought it would be good to get in front of you