search

SecKatie / ha-wyzeapi

Home Assistant Integration for Wyze devices.
722 stars 112 forks source link

Update thermostat enum #562

Closed brg468 closed 2 months ago

brg468 commented 3 months ago

Modify the name of the HVACMode enum to differentiate from the HA enum of the same name.

Untested...But was reported to fix the issue by a helpful tester.

Hope to fix https://github.com/SecKatie/ha-wyzeapi/issues/560

JoeSchubert commented 3 months ago

I'm confused on the point of this. All it's doing is changing the existing HVACMode to use an import alias.

The changes shouldn't really make any difference from the existing code before these changes.

JoeSchubert commented 3 months ago

Went back and read the issue that you linked. If Wyze changed the valid values so 'off' is no longer accurate, this would need to be corrected in this enums: https://github.com/SecKatie/wyzeapy/blob/6ea8ae5c38c5247a1a534a127fbf9b986735157d/src/wyzeapy/services/thermostat_service.py#L16

Not here.

brg468 commented 3 months ago

I’m not entirely sure why either, all I know is that HA was trying to set modes with our enum’s instead of theirs of the same name. Because of the order if imports ours were being used throughout.

JoeSchubert commented 3 months ago

I’m not entirely sure why either, all I know is that HA was trying to set modes with our enum’s instead of theirs of the same name. Because of the order if imports ours were being used throughout.

Ohhh. That makes a little bit more sense.

It's trying to use the values from https://github.com/home-assistant/core/blob/5b9f40b0f06921db93650002a84c2dec78d61733/homeassistant/components/climate/const.py#L14

But that class is a StrEnum instead of just an enum. It's probably a good idea to avoid using names that could interact/interfere with HA's builtin names anyway.

The reason for the conflict is that their HVACmode is imported earlier than ours. Normally you should see a warning in the log about the reassignment of the import

I see no issues with this change. Good catch!

brg468 commented 3 months ago

👍 thanks!

If only I'd noticed the conflict when I made the initial changes 😄 but thats the beauty of not having half these devices to test on first.

zacheryrodgers commented 3 months ago

can we please merge this asap?

brg468 commented 3 months ago

I can merge but I don’t know how to push a new version, so I’m not sure that’s helpful…