Skip to content
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

Move event socket to Appliances Manager #72

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

lllucius
Copy link

@lllucius lllucius commented Jan 8, 2025

This was a merge of @NodeJSmith's move_event_socket_to_app_manager branch.

First step of merging my changes.

This was a merge of @NodeJSmith's move_event_socket_to_app_manager
branch.
@NodeJSmith
Copy link
Contributor

Glad to see my branch had some merit! Never got around to finishing that up. Let me know if you have any questions on my changes or if you like another set of eyes (or hands) for any changes. I might try to spend a bit more time contributing if the code base is getting cleaned up

whirlpool/appliance.py Outdated Show resolved Hide resolved
@abmantis abmantis marked this pull request as draft January 8, 2025 18:25
@lllucius
Copy link
Author

lllucius commented Jan 8, 2025

Glad to see my branch had some merit! Never got around to finishing that up. Let me know if you have any questions on my changes or if you like another set of eyes (or hands) for any changes. I might try to spend a bit more time contributing if the code base is getting cleaned up

I'm glad you did it. I was going through the open issues and found your link there so it saved me a fair amount of work.

@lllucius lllucius marked this pull request as ready for review January 10, 2025 00:14
Copy link
Owner

@abmantis abmantis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes. Left a few comments but overall this looks good!

whirlpool/appliancesmanager.py Outdated Show resolved Hide resolved
whirlpool/appliancesmanager.py Outdated Show resolved Hide resolved
whirlpool/appliancesmanager.py Outdated Show resolved Hide resolved
whirlpool/appliancesmanager.py Outdated Show resolved Hide resolved
whirlpool/appliancesmanager.py Outdated Show resolved Hide resolved
whirlpool/oven.py Outdated Show resolved Hide resolved
whirlpool/oven.py Outdated Show resolved Hide resolved
whirlpool/oven.py Outdated Show resolved Hide resolved
whirlpool/oven.py Outdated Show resolved Hide resolved
whirlpool/types.py Outdated Show resolved Hide resolved
@lllucius
Copy link
Author

Sorry...work got in the way. Where do we stand on these PRs? Was there something else that I was supposed to be changing?

@abmantis
Copy link
Owner

Sorry...work got in the way. Where do we stand on these PRs? Was there something else that I was supposed to be changing?

There are a few comments that are still not addressed

@lllucius
Copy link
Author

Oh, geez. I missed a lot of them. Will work on them tonight.

@lllucius lllucius requested a review from abmantis January 18, 2025 04:04
Copy link
Owner

@abmantis abmantis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still a few unaddressed comments from the previous review. Please make sure to address all comments before requesting a re-review.

whirlpool/appliancesmanager.py Outdated Show resolved Hide resolved


@dataclass
class ApplianceData:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class ApplianceData:
class ApplianceInfo:

since we already call appliance data to the actual data (temperature, mode, etc), lets name this Info to reduce ambiguity.

whirlpool/appliance.py Outdated Show resolved Hide resolved

self._session: aiohttp.ClientSession = session

def register_attr_callback(self, update_callback: Callable):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to move this down?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean for me to do here. Just let me know and I'll get 'er done.

"Cache-Control": "no-cache",
}

def _set_attribute(self, attribute, value, timestamp):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to move this down?

whirlpool/backendselector.py Outdated Show resolved Hide resolved
whirlpool/backendselector.py Outdated Show resolved Hide resolved
@@ -170,32 +168,29 @@ def get_state(self):
return None

async def set_timer(self, timer_time: int, operation=KitchenTimerOperations.Start):
await self._appliance.send_attributes(
await self.send_attributes(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unresolved

whirlpool/oven.py Outdated Show resolved Hide resolved
def _update_appliance_attributes(self, attrs: dict[str, str], timestamp: str):
for attr, val in attrs.items():
if self.has_attribute(attr):
self._set_attribute(attr, str(val), timestamp)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self._set_attribute(attr, str(val), timestamp)
self._set_attribute(attr, val, timestamp)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I "think" these are "reversals" of earlier review points???

@lllucius
Copy link
Author

There are still a few unaddressed comments from the previous review. Please make sure to address all comments before requesting a re-review.

I really thought I'd addressed them all! I have no idea why I'm not seeing everything!

I realized that HA will not be adequate (yet) for my purposes, so I had to revert to writing an Alexa skill to interface directly to the dryer. Nearly done with that, so I'll address the above issues shortly...probably in a couple of days.

@abmantis
Copy link
Owner

I really thought I'd addressed them all! I have no idea why I'm not seeing everything!

image

Maybe you missed the button to expand? You wouldn't be the first eheh.

I realized that HA will not be adequate (yet) for my purposes, so I had to revert to writing an Alexa skill to interface directly to the dryer. Nearly done with that, so I'll address the above issues shortly...probably in a couple of days.

Oh. Why not connect HA to Alexa?

@lllucius
Copy link
Author

I really thought I'd addressed them all! I have no idea why I'm not seeing everything!

image

Maybe you missed the button to expand? You wouldn't be the first eheh.

I realized that HA will not be adequate (yet) for my purposes, so I had to revert to writing an Alexa skill to interface directly to the dryer. Nearly done with that, so I'll address the above issues shortly...probably in a couple of days.

Oh. Why not connect HA to Alexa?

Yepper, did that. But, it felt a little clunky and since Alexa doesn't support the same entity types that HA does, I'd need to fiddle around with "forcing" this integration. So, I figured a skill built for purpose would be best, at least until I can get my hands on a decent voice assistant for HA. I'm not opposed to building one myself, but there's a couple of decent sounding ones coming out and I'd rather have someone else build it for me. :-)

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.

3 participants