dolezsa / thermal_comfort

Thermal Comfort sensor for HA (absolute humidity, heat index, dew point, thermal perception)
Other
590 stars 110 forks source link

RFC: Adding non legacy yaml configuration #84

Closed rautesamtr closed 2 years ago

rautesamtr commented 2 years ago

Since there's Architecture Decision Record ADR0007 for home assistant this means the current configuration used in thermal comfort is considered legacy yaml configuration. So it would make sense to have a non configuration at some point. This can be done in a non breaking way supporting both configuration types. The old configuration will be supported as minimum till the next breaking version or even longer should maintenance stay at a minimum. See home assistants template integration for a partial migration to the new yaml configuration style. Or this work in progress PR https://github.com/home-assistant/core/pull/63840 for the command line integration.

Biggest differences would be:

Are there any suggestions, questions or objections?

If there's any idea/reason for a breaking change of the configuration this would be the best time since both old and new configuration can coexist so it would be possible to implement breaking changes just in the new configuration type.

dolezsa commented 2 years ago

sooner or later we will have to make this change, and from my point of view, especially if it is possible, as you write, with support both types, it would be best to do it so as soon as possible. also the possibility of configuration via ui would be nice to have imho.

rautesamtr commented 2 years ago

There's one thing I'd personally like to break but is probably not the most welcome change. Imho it would be nice if we could change the sensor name from singlewords to seperate_words. e.g. This would make them more readable easier to convert to other strings. For example one could easily generate the default friendly name of the sensor type. And if we do this we could take another look at the string for the perception sensor type. Which could be changed to thermal_perception.

https://github.com/dolezsa/thermal_comfort/blob/393e9bfdc0c89e1e3a5dbc1cb20a3cde46ee777c/custom_components/thermal_comfort/sensor.py#L55-L65

This could also be implemented with a mapper that accepts the old strings to keep everything backward compatible for now.

it would be best to do it so as soon as possible. also the possibility of configuration via ui would be nice to have imho.

Sounds good. We probably should add both in the same release cycle since the both build on common stuff that needs to be added. @IATkachenko has already done a lot of the frontend stuff in #86 and with a bit of polish it should be a good foundation to build upon.

So basically having both on the next feature release would offer users the choice to either migrate to the frontend config, new yaml config or stick with their old config.

dolezsa commented 2 years ago

just do it