Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

weather integration: change partlycloudy to partly-cloudy #321

Closed
Mariusthvdb opened this issue Dec 9, 2019 · 9 comments
Closed

weather integration: change partlycloudy to partly-cloudy #321

Mariusthvdb opened this issue Dec 9, 2019 · 9 comments

Comments

@Mariusthvdb
Copy link

Mariusthvdb commented Dec 9, 2019

Context

The issue motivating this decision, and any context that influences or constrains the decision.

following discussion on #29569 this aims to use partly-cloudy all through HA's weather integrations and backend. Will Pr all integrations and documentation accordingly

Proposal

The change that you're proposing.

To change the weather integration default state partlycloudy to partly-cloudy because it is the only condition left without a dash between single words.
This would also make it easier to create templates based on these condition states, eg

icon_template:
  mdi:weather-{{states.('weather.home_darksky')}}

and would make the HA syntax consistent.

Consequences

What becomes easier or more difficult to do and any risks introduced by the change that will need to be mitigated.

As said it becomes much easier to create templates based on the conditions, (because of no exception without -), and as a consequence follow suit with the mdi icon repository which also holds a dash between all single words. Everywhere.

Put differently, this change would only hold advantages, and no disadvantages at all. A simple change in the mentioned integrations and documentation would suffice.

It is in fact quite remarkable this hasn't been implemented earlier, but, given the fact a few Mdi icons have been added lately, it is a fine moment to repair this Homeassistant wide now.

@balloob
Copy link
Member

balloob commented Dec 11, 2019

It's not the aim of Home Assistant to be consistent with Material Design Icons.

It was a mistake to blindly adopt the MDI icon names as our conditions, as that has caused it to create the assumption that they should be aligned to begin with.

This issue would be a breaking change for many people and it would have 0 added value in the backend or in the frontend.

For your templates, you can just create a look up table.

{% set weather_icons = {
  "partlycloudy": "mdi:bla",
  "sunny": "mdi:sunny"
} %}

Icon is {{ weather_icons["partlycloudy"] }}

@Mariusthvdb
Copy link
Author

Mariusthvdb commented Dec 11, 2019

being a breaking change hasn't really stopped thinking in HA development before...
Also, if this is all fixed in the backend, (as I was told to do so when I asked if this would be a subject for the architecture issues and the answer was no...) how would it effect many people, if you say it never gets to the frontend.

I understand it might not have been the aim to be consistent with MDI.
Still HA proudly shows:
Schermafbeelding 2019-12-11 om 10 57 42

And, since this has been de facto because of it, why not let this tiny fix make it complete on all states.

Even without that, why keep an anomaly from the past, if it would be as simple as to add a single - in 1 single state field, and announce that in the docs, and even a breaking change if necessary.

Sure, we can set templates, as a matter of fact we are practically forced to do so because of this one single exception, and some varying states.
Using this to find our way between all different states of the many platforms:

      weather_icon:
        entity_id: sensor.dark_sky_icon, weather.home_darksky, weather.woensdrecht
        friendly_name_template: >
          {{states('sensor.weather_icon').split('weather')[1]|replace('-',' ')|title}}
        value_template: >
          {% set mapper_icon =
            {'partly-cloudy-night': 'night-partly-cloudy'} %}
          {% set mapper_br =
            {'pouring': 'pouring',
             'lightning-rainy': 'lightning-rainy',
             'snowy-rainy': 'snowy-rainy'} %}
          {% set mapper_ds =
            {'sunny': 'sunny',
             'clear-night': 'night',
             'rainy': 'rainy',
             'snowy': 'snowy',
             'snowy-rainy': 'snowy-rainy',
             'windy': 'windy',
             'fog': 'fog',
             'cloudy': 'cloudy',
             'partlycloudy': 'partly-cloudy',
             'hail': 'hail',
             'lightning':'lightning'} %}
          {% set icon = states('sensor.dark_sky_icon') %}
          {% set woensdrecht = states('weather.woensdrecht') %}
          {% set dark = states('weather.home_darksky') %}
          {% set weather = mapper_icon[icon] if icon in mapper_icon else 
                           mapper_br[woensdrecht] if woensdrecht in mapper_br else 
                           mapper_ds[dark] if dark in mapper_ds else 'sunny-alert' %}
            mdi:weather-{{weather}}

        icon_template: >
          {{states('sensor.weather_icon')}}

speaks for itself...
Seeing what's necessary to align the various weather integrations icon/state mappings I really can't understand the hesitance you express here to repair this easy one, especially since it truly is the 1 single state left where 2 words are concatenated.

That HA syntax aspect could be reason alone to do so?

@balloob
Copy link
Member

balloob commented Dec 11, 2019

We still use material design icons and so credit them. However we use them in the frontend, we don't use them to drive our architecture design.

@MatthewFlamm
Copy link
Contributor

I think that this template below also doesn't work for clear-night and exceptional, so this motivation doesn't seem clear either.

icon_template:
  mdi:weather-{{states.('weather.home_darksky')}}

@Mariusthvdb
Copy link
Author

of course, I expected that ;-) I have dealt with that condition in my bigger template above as you can see. Had to.

It merely underlines the plethora of different states the various HA weather platforms and sensors hold.
Mine is a small effort trying to get rid of one of the more obvious 'errors' in the current HA weather system, without creating a true breaking change of any importance

As a matter of fact, it is so small it doesn't even require an architecture discussion in the first place according to https://developers.home-assistant.io/docs/en/entity_index.html#changing-the-entity-model

The choice btw for 'exceptional' further underlines that. It was an arbitrary choice, and could just as easily have been tornado if the component returns tornado... Again, the icon would have been easy in that case.

The point should be clear now I would think.

Really sorry if this would not pass, as said, such a small thing to make HA syntax consistent, no issue at all for breaking changes, only benefits. Probably small too, but hey, if you don't value the small (old dutch saying)

I'll leave at this, be glad to work at it if green-lighted. If not, well really sorry of course ;-)
cheers!

@foreign-sub
Copy link

Hi,
I was wondering, should these names follow the style guidelines ?
If so, it seems appropriate to rename multi-word names to lower_case_with_underscores, so partlycloudy would become partly_cloudy.

@fabaff
Copy link
Member

fabaff commented Dec 12, 2019

If we are going to change it then partly_cloudy.

@Mariusthvdb
Copy link
Author

with aforementioned reason by @foreign-sub : of course that would be even better.
No reason to post/demand 'enforce' strict style guidelines if not following them..
use 'for uncompromised code formatting'

Would require a little more work ;-)

@balloob
Copy link
Member

balloob commented Mar 21, 2020

Not going to implement, not worth the breaking change.

@balloob balloob closed this as completed Mar 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants