Climate Set Temperature Signature and behavior

[albertomontesg]

This change proposes some improvements on how the Climate base entity class handles the set temperature endpoint.

The problem

Some of the problems I have observed with the current implementation are:

1. The arguments passed to the method are a dictionary of keyword arguments (**kwargs) that each climate implementation needs to unpacked using some constants and they need to handle their optional nature.
2. Depending on the ClimateEntityFeature, we expect it to pass just the temperature or the rage of temperatures depending whether the entity supports TARGET_TEMPERATURE or TARGET_TEMPERATURE_RANGE.

This can be confusing and leads to different Climate implementations creating different handling given the free form parameters passed to the method (previous discussion: home-assistant/core#114290 (comment)).

Proposal

The proposal of changes are:

1. Hardcode the signature of the (async_)set_temperature methods to specify which are the arguments names and types.
2. At async_service_temperature_set implement some logic that would conditional to the supported features on the entity, populate only the arguments on the method that are needed (if TARGET_TEMPERATURE pass the temperature argument and if TARGET_TEMPERATURE_RANGE pass the target_temp_(high|low) arguments).

A prototype of this change can be found here: home-assistant/core@92d867d

Backwards Compatibility and Breaking Changes consideration

The change on the signature is backwards compatible, as the only call the set_temperature endpoints in the Entity is done by the service method, and that guarantees (through the Service schema) the parameters that are being passed. Also having the children classes of ClimateEntity defining the method with **kwargs is backwards compatible (logic wise). The only challenge here is that mypy will complain about signature mismatch between parent and child which would need to be addressed.

Regarding the update in the logic on the async_service_temperature_set, it should not make any breaking changes on the child implementations. Most of them they were already making checks (and even raising exceptions), but the expectation is that climates that support range to consume a range, and supporting a target temperature to consume its value.

With the early prototype and running through some tests for climate implementations, I have not seen any breakage whatsoever (this needs to be more exhaustive)

Alternatives Considered

There is a good practice in software engineering that is says: make invalid states unrepresentable. Right now the set_temperature signature looks like this:

def set_temperature(
        temperature: float | None = None,
        target_temp_high: float | None = None,
        target_temp_low: float | None = None,
        hvac_mode: HVACMode | None = None,
) -> None:

But this signature allows some states to be represented that are invalid which forces all child implementations to handle the possibility of having an invalid state passed. The invalid state I am referring too is for example where no temperature argument has been passed, or when only high temp has been passed but no low (being invalid as of the official documentation).

Option 1

Some alternative could be to update the method signature as follows:

class TemperatureRange(NamedTupe):
  high: float
  low: float

def set_temperature(
  temperature: float | None = None,
  temperature_range: TemperatureRange | None = None,
  hvac_mode: HVACMode | None = None,
) -> None:

To go even further (to avoid not having any temperature being set and provide the guarantee that some temperature is passed to child implementations:

class TemperatureRange(NamedTupe):
  high: float
  low: float

@dataclasses.dataclass
class TargetTemperature:
  target: float | None = None
  range: TemperatureRange | None = None

  def __post_init__(self);
    if self.target is None and self.range is None:
      raise ValueError(...)

def set_temperature(
  target_temperature: TargetTemperature, # This is guaranteed that at least target or range is passed.
  hvac_mode: HVACMode | None = None,
) -> None:

Option 2 (preferred)

Given that the service call either enforces that temperature or temperature range are passed by the user, a better alternative would be to split the method into 2 methods, each targeting each of the options:

class MyClimate(ClimateEntity):
  def set_target_temperature(self, temperature: float, hvac_mode: HVACMode | None = None) -> None:
    # Logic to specify only the target temperature.

  def set_target_temperature_range(self, temperature_low: float, temperature_high: float, hvac_mode: HVACMode | None = None) -> None:
    # Logic to set the temperature range.

Unfortunately this changes would require to explicitly update all the climate sub-entities (and could be a breaking change for custom components). Given that we would need to update all the child Climate signatures to make mypy happy, making the change with a more correct signature might be a small overhead on top of it.

Happy to hear feedback from this.

[rokam]

I do like the idea of a temperature class to receive the values. With this, we can also assume a target temp or a target range based on the passed information. Eg. Range of 20-24 => temp 22 (avg).

That way, we avoid user input error throughout the service input.