home-assistant / architecture

Repo to discuss Home Assistant architecture
313 stars 100 forks source link

Remove humidity as a required method for implementing a weather entity #485

Closed MatthewFlamm closed 1 year ago

MatthewFlamm commented 3 years ago

Context

As requested in https://github.com/home-assistant/core/pull/45095, this is an architecture issue, although it is unclear to me whether the code base or the documentation is the definitive source for the entity model. The dev docs suggest that humidity is an optional value, but the weather integration requires each subsequent weather integration to implement humidity method. In PR https://github.com/home-assistant/core/pull/44807, there is no humidity data, so the integration must implement a method that returns None.

Proposal

I would propose making the code base be faithful to the developer documentation and allow weather integrations to omit the humidity value if it is unavailable from their data source.

Consequences

It is easier for integrations without humidity data to implement and removes unnecessary code from those integrations. One negative is that it would allow weather integrations without humidity data, although this is currently supported in the documentation. No current weather integration returns None for this value, but there is a pending PR for one.

dgomes commented 3 years ago

Actually there are 2: https://github.com/home-assistant/core/pull/45031

balloob commented 3 years ago

From https://github.com/home-assistant/core/pull/3546#discussion_r80850910 I understand that the dev docs are wrong.

Screenshot 2021-01-13 at 16 33 32@2x

The problem with making too many things optional is that it will be impossible to show a nice UI, as we need to deal with all the possible permutations. Doesn't mean we can consider this.

I'm surprised Tado weather doesn't have humidity. It really is the only one?

MatthewFlamm commented 3 years ago

Tado is the only (proposed) weather integration without humidity implemented. I do not know if it is truly missing or just left off in Tado, maybe @Noltari can comment. The other case is the one @dgomes linked where a template weather entity would now not require humidity to be provided, but that one could be reworked either way I think.

Humidity isn't even shown on my UI card. Maybe it depends on the exact availability of the other attributes (and card size?), but this highlights how uncritical it is viewed on the UI today. I think that condition and temperature in both observation and forecast are the only essential pieces as evidenced in the current UI.

I don't really have a strong stake here. I'm just trying to get to the bottom of how to clear up either the code or the dev docs so that the expectations are clear. Also, if Tado is truly missing humidity, I assume that means that PR would have to be rejected if humidity is required, so there are consequences on that side too.

Noltari commented 3 years ago

Tado is the only (proposed) weather integration without humidity implemented. I do not know if it is truly missing or just left off in Tado, maybe @Noltari can comment. The other case is the one @dgomes linked where a template weather entity would now not require humidity to be provided, but that one could be reworked either way I think.

Humidity isn't even shown on my UI card. Maybe it depends on the exact availability of the other attributes (and card size?), but this highlights how uncritical it is viewed on the UI today. I think that condition and temperature in both observation and forecast are the only essential pieces as evidenced in the current UI.

I don't really have a strong stake here. I'm just trying to get to the bottom of how to clear up either the code or the dev docs so that the expectations are clear. Also, if Tado is truly missing humidity, I assume that means that PR would have to be rejected if humidity is required, so there are consequences on that side too.

@MatthewFlamm humidity isn't provided by Tado: https://github.com/Noltari/home-assistant-core/blob/tado-weather/tests/fixtures/tado/weather.json

{
   "outsideTemperature": {
      "celsius": 7.46,
      "fahrenheit": 45.43,
      "precision": {
         "celsius": 0.01,
         "fahrenheit": 0.01
      },
      "timestamp": "2020-12-22T08:13:13.652Z",
      "type": "TEMPERATURE"
   },
   "solarIntensity": {
      "percentage": 2.1,
      "timestamp": "2020-12-22T08:13:13.652Z",
      "type": "PERCENTAGE"
   },
   "weatherState": {
      "timestamp": "2020-12-22T08:13:13.652Z",
      "type": "WEATHER_STATE",
      "value": "FOGGY"
   }
}

Only temperature, weather state and solar percentage are provided.

BTW, humidity is only shown in the weather card if selected as the second info attribute (at least with the standard weather card).

bouwew commented 3 years ago

Just my 2 cent, why should there be a Tado weather? Tado is not a weather provider, it reuses data available from an existing, real weather provider. Just create sensors for the info that you want to show. If we end up doing this, there will be a Nest weather, a Plugwise weather, etc., do we really want to go this way?

MatthewFlamm commented 3 years ago

Tado is not a weather provider, it reuses data available from an existing, real weather provider.

This is off topic, but this logic would preclude many, many weather providers who buy their weather data from central resources.

44807 has now removed the weather platform, but this issue still stands on whether to correct the dev docs or the codebase.

dgomes commented 3 years ago

I think we should not remove humidity from the weather platform, Temperature/Humidity are the common denominator in any basic weather service and even on any basic weather station.

Dev docs should be updated.

frenck commented 1 year ago

This architecture issue is old, stale, and possibly obsolete. Things changed a lot over the years. Additionally, we have been moving to discussions for these architectural discussions.

For that reason, I'm going to close this issue.

../Frenck