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

Add Huawei LTE binary sensor support, mobile connection sensor #28226

Merged
merged 3 commits into from Dec 1, 2019

Conversation

@scop
Copy link
Contributor

scop commented Oct 26, 2019

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:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@project-bot project-bot bot added this to Needs review in Dev Oct 26, 2019
@project-bot project-bot bot moved this from Needs review to By Code Owner in Dev Oct 26, 2019
@scop scop mentioned this pull request Oct 26, 2019
2 of 2 tasks complete
@property
def icon(self):
"""Return binary sensor icon."""
return "mdi:signal" if self.is_on else "mdi:signal-off"

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 26, 2019

Member

Device class entity icons should not be overridden.

This comment has been minimized.

Copy link
@scop

scop Oct 26, 2019

Author Contributor

This is already being done for pretty much all Huawei LTE sensors and switches.

This comment has been minimized.

Copy link
@scop

scop Oct 26, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 27, 2019

Member

I'll ask for a second opinion.

This comment has been minimized.

Copy link
@scop

scop Nov 2, 2019

Author Contributor

Any progress, can we move forward here?

This comment has been minimized.

Copy link
@scop

scop Nov 16, 2019

Author Contributor

Do we have some conclusion here?

This comment has been minimized.

Copy link
@balloob

balloob Nov 25, 2019

Member

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

This comment has been minimized.

Copy link
@scop

scop Nov 27, 2019

Author Contributor

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:

  1. use connectivity device class, override icon (this is what the PR does now)
  2. drop connectivity device class altogether, keep the icon I like
  3. use connectivity device class, remove icon override (this is what Martin suggested)
  4. 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.

This comment has been minimized.

Copy link
@balloob

balloob Nov 27, 2019

Member

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.

This comment has been minimized.

Copy link
@scop

scop Nov 28, 2019

Author Contributor

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.

Dev automation moved this from By Code Owner to Review in progress Oct 26, 2019
@project-bot project-bot bot moved this from Review in progress to Second opinion wanted in Dev Oct 27, 2019
Dev automation moved this from Second opinion wanted to Reviewer approved Nov 28, 2019
Copy link
Member

MartinHjelmare left a comment

Looks good!

@balloob balloob merged commit 5c8a8a6 into home-assistant:dev Dec 1, 2019
9 checks passed
9 checks passed
CI #20191128.30 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
Dev automation moved this from Reviewer approved to Done Dec 1, 2019
@scop scop deleted the scop:huawei-lte-binary-sensor branch Dec 1, 2019
@lock lock bot locked and limited conversation to collaborators Dec 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
4 participants
You can’t perform that action at this time.