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

Projects
None yet
3 participants
@amelchio
Copy link
Contributor

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

This comment has been minimized.

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,

This comment has been minimized.

Copy link
@amelchio

amelchio Apr 9, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Apr 9, 2019

Member

I don't have a better solution here.

@amelchio

This comment has been minimized.

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 amelchio referenced this pull request Apr 9, 2019

Merged

Netgear LTE binary sensors #9178

2 of 2 tasks complete
@amelchio

This comment has been minimized.

Copy link
Contributor Author

amelchio commented Apr 9, 2019

Thank you. I added the documentation update now.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Apr 9, 2019

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

12 checks passed

ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
ci/circleci: test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: test 3.7 Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 55c8417...b20a388
Details
codecov/project 93.83% (target 90%)
Details

@wafflebot wafflebot bot 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
You can’t perform that action at this time.