search

snicker / zwift_hass

Zwift Sensor Integration for HomeAssistant
53 stars 13 forks source link

Updates seraphimserapis #31

Closed SeraphimSerapis closed 2 years ago

SeraphimSerapis commented 2 years ago

Tested with hassfest and works fine for my installation and addresses #30 and #29.

snicker commented 2 years ago

Looks good, sorry for not seeing this until now! I will likely be able to merge later (thank you for the formatting improvements as well)

one concern- I can't recall which version of Home Assistant switched from device_state_attributes to extra_state_attributes, but we either need to add both (and return the same values) temporarily, or at a minimum require a minimum version of HA in the manifest (and hacs.json). This is common practice for any breaking changes so users on older versions of HA cannot update to the latest version of the integration

SeraphimSerapis commented 2 years ago

Do you have a preference? I'm fine either way with a slight preference towards raising the minimum version required to run this component.

snicker commented 2 years ago

agreed, I am okay with that approach to raise minimum version.

On Feb 6, 2022, at 9:01 AM, Tim Messerschmidt @.***> wrote:

 Do you have a preference? I'm fine either way with a slight preference towards raising the minimum version required to run this component.

— Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you commented.

SeraphimSerapis commented 2 years ago

I merged your changes/fixes and increased the min version as agreed. Let me know if there's anything else you'd like to see addressed/changed.

snicker commented 2 years ago

Thanks @SeraphimSerapis ! all merged now.