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

Binary sensors for netgear_lte #22902

Merged
merged 5 commits into from Apr 9, 2019

Conversation

amelchio
Copy link
Contributor

@amelchio amelchio commented Apr 8, 2019

Description:

Follow-up to #22558 where @rohankapoorcom pointed out that some of the sensors should have been binary sensors. Since #22558 is not released yet, this is not a breaking change.

Most of the changes here are due to moving shared methods to a base class in the component. The rest is a binary_sensor implementation, mostly copy/pasting the existing sensor implementation. I did not split into several PRs mostly because the clock is ticking on the next beta.

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#9178

Example entry for configuration.yaml (if applicable):

netgear_lte:
  - host: !secret lb2120_hostname
    password: !secret lb2120_password
    binary_sensor:
      monitored_conditions:
        - wire_connected
        - mobile_connected
        - roaming

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox.
  • There is no commented out code in this PR.

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 (example).
  • New dependencies have been added to requirements in the manifest (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

@codecov
Copy link

codecov bot commented Apr 8, 2019

Codecov Report

Merging #22902 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev   #22902   +/-   ##
=======================================
  Coverage   93.83%   93.83%           
=======================================
  Files         448      448           
  Lines       36574    36574           
=======================================
  Hits        34321    34321           
  Misses       2253     2253
Impacted Files Coverage Δ
homeassistant/components/mqtt/cover.py 94.07% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55c8417...b20a388. Read the comment docs.

BINARY_SENSOR_MOBILE_CONNECTED = 'mobile_connected'

BINARY_SENSOR_CLASSES = {
'roaming': None,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in doubt about the proper device class to use here. The UI currently shows a checkmark when roaming and that seems odd. However, "roaming" is the terminology normally used so I see no way of reversing the naming without causing confusion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a better solution here.

@amelchio
Copy link
Contributor Author

amelchio commented Apr 9, 2019

@MartinHjelmare I don't normally ask for reviews but it would be good to get this in before the beta and it is mostly small changes to code that you already reviewed, so if you have the time ...

@amelchio
Copy link
Contributor Author

amelchio commented Apr 9, 2019

Thank you. I added the documentation update now.

@MartinHjelmare
Copy link
Member

I've commented on the docs PR. Go ahead and merge when everything is ready. 👍

@amelchio amelchio merged commit 58ec77b into home-assistant:dev Apr 9, 2019
@ghost ghost removed the in progress label Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants