Add Huawei LTE binary sensor support, mobile connection sensor#28226
Conversation
| @property | ||
| def icon(self): | ||
| """Return binary sensor icon.""" | ||
| return "mdi:signal" if self.is_on else "mdi:signal-off" |
There was a problem hiding this comment.
Device class entity icons should not be overridden.
There was a problem hiding this comment.
This is already being done for pretty much all Huawei LTE sensors and switches.
There was a problem hiding this comment.
To elaborate:
If I leave this out, the default icon for a connectivity class binary sensor is something clearly related to wired connections, which is bad for a mobile connectivity sensor.
It would be also very different from the closely related mobile data switch, which would be bad too. Now it's the same, which is consistent and good.
If I remove it from mobile data switch as well, then its icon becomes the lightning one which isn't a good one for a mobile connectivity switch, and it would also be very different from any choices I have for the mobile connection sensor, so things would get even worse.
Note also that this icon is solely for the mobile connection sensor, it's not in the base binary sensor class. The method docstring could be improved to say it's the icon for mobile connection sensor, not binary sensor. Done now.
There was a problem hiding this comment.
I'll ask for a second opinion.
There was a problem hiding this comment.
Any progress, can we move forward here?
There was a problem hiding this comment.
Do we have some conclusion here?
There was a problem hiding this comment.
Device classes are not just icons, they also define what data is represented. It means it impacts how systems that built on top of Home Assistant represent it, like Google, Alexa or Almond.
So if the icon does not represent the device class properly, we should consider updating the device class icon
There was a problem hiding this comment.
For generic connectivity, I don't think there's anything wrong per se with the current icon. But for mobile connectivity, it's not a good one. And I don't want to get into the bikeshed whether one better describing mobile connectivity (e.g. the one I used here) would be acceptable for all connectivity.
I guess a kind of a sweet spot would be to be able to define a mobile connectivity device class that would inherit from the generic connectivity device class and have a different icon (possibly also some other differences/additional behavior or data sometime), although I suppose (without digging around) that that's not an option with the current device class implementation. And I think it's out of the scope for this pull request, and neither would I want to wait for it to come up before getting this in.
So at the moment my practical choices boil down to:
- use connectivity device class, override icon (this is what the PR does now)
- drop connectivity device class altogether, keep the icon I like
- use connectivity device class, remove icon override (this is what Martin suggested)
- wait for a new device class with the icon I like to be introduced
My favourite, by far, would be 1, then 4 sometime later when/if it's available. Going through 2 or 3 eventually to 4 when it's around would in my opinion in the meantime introduce either suboptimal user experience with the icon as elaborated earlier, or possible loss of some functionality caused by a missing device class. Just waiting for 4 before introducing this is an unnecessary delay.
If this can't go in its current form (i.e. 1 above) I'm currently torn between removing the icon and getting this approved, or just keeping the implementation to myself until 4 comes up. But I'd appreciate a clear decision/conclusion.
There was a problem hiding this comment.
I prefer to go with 2 while we wait till 4 is available. The device classes are not extensively used. I think the only thing that I know that uses it now is Almond, for which you'll soon be able to ask if X is connected. Changing a device class is technically a breaking change, adding one is not.
We don't differentiate or inherit device classes, because the extra complexity will cause pain in other systems relying on it.
There was a problem hiding this comment.
Device class dropped.
At least in one sense inheriting a device class will cause less complexity to consumers than adding a new one. For example if "mobile connectivity" was extended from "connectivity", systems interesting in any kind of connectivity would just have to check for "connectivity" instead of both.
MartinHjelmare
added
the
second-opinion-wanted
MartinHjelmare
removed
the
second-opinion-wanted
4 participants
Description:
Per subject.
Related issue (if applicable): fixes #
Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#10996
Checklist:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
python3 -m script.hassfest.requirements_all.txtby runningpython3 -m script.gen_requirements_all..coveragerc.If the code does not interact with devices: