Is logic in service.py's set_hold for setting heat_hold_temp or cool_hold_temp valid?

[chrisweis]

I'm questioning this portion of the code in service.py:

2701 if hold_climate_ref is None and (
2702 cool_hold_temp is None OR heat_hold_temp is None
2703 ):
2704 raise ValueError(
2705 'hold_climate_ref is None. cool_hold_temp and heat_hold_temp must '
2706 'not be None.'
2707 )

Should that "or" in line 2702 instead be "and"?

[sherif-fanous]

This is the current code.

        if (
            cool_hold_temp is None
            and heat_hold_temp is None
            and hold_climate_ref is None
        ):
            raise ValueError(
                'cool_hold_temp, heat_hold_temp, and hold_climate_ref must not all '
                'be None. Either cool_hold_temp and heat_hold_temp must not be None '
                'or hold_climate_ref must not be None'
            )

What are you thinking it should be and why? The current code ensures that at least one of cool_hold_temp, heat_hold_temp, or hold_climate_ref is not None. If all 3 are None then a ValueError is raised.

[sherif-fanous]

This is the following condition

        if hold_climate_ref is None and (
            cool_hold_temp is None or heat_hold_temp is None
        ):
            raise ValueError(
                'hold_climate_ref is None. cool_hold_temp and heat_hold_temp must '
                'not be None.'
            )

This ensures that neither cool_hold_temp nor heat_hold_temp are None if hold_climate_ref is None. i.e. You must pass a value for both cool_hold_temp and heat_hold_temp if you don't pass a value for hold_climate_ref

[chrisweis]

Thanks @sfanous, didn't expect both heat and cool temp to be required since usually we only tend to pick one as a user, but if the API requires both, it now makes more sense to me. Thanks for the quick reply!

[sherif-fanous]

@chrisweis To be fair the documentation doesn't mention that both are required. I no longer recall why I set this validation.

Feel free to clone the repo make the required changes and test it. It it works then please submit a PR.

[sherif-fanous]

Actually let me take that away I just found references to why I have this validation

https://www.ecobee.com/home/developer/api/examples/ex5.shtml

Here are the important excerpts

Note that when creating a temperature hold, both heat and cool temperatures must be specified.

It is good practice to set the heat and cool hold temperatures to be the same

And the sample payload in the example

{
  "selection": {
    "selectionType":"registered",
    "selectionMatch":""
  },
  "functions": [
    {
      "type":"setHold",
      "params":{
        "holdType":"nextTransition",
        "heatHoldTemp":700,
        "coolHoldTemp":700
      }
    }
  ]
}