Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

Lash-L
Copy link
Collaborator

@Lash-L Lash-L commented Oct 2, 2025

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

@Lash-L Lash-L requested a review from allenporter October 2, 2025 01:15
@Lash-L Lash-L marked this pull request as draft October 2, 2025 01:15
Copy link

github-actions bot commented Oct 2, 2025

🤖 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):
Copy link
Collaborator Author

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

Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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)
Copy link
Collaborator Author

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

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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?

Copy link
Collaborator Author

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?

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.