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

Added a Taps Aff binary sensor #7880

Merged
merged 8 commits into from Jun 4, 2017
Merged

Conversation

bazwilliams
Copy link
Contributor

@bazwilliams bazwilliams commented Jun 3, 2017

Description:

This adds a binary sensor using the 'tapsaff.co.uk' service to indicate whether it is 'taps aff' in a specified location.

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

Example entry for configuration.yaml (if applicable):

binary_sensor:
  - platform: tapsaff
    location: Glasgow

Checklist:

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

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

@mention-bot
Copy link

@bazwilliams, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @robbiet480 to be potential reviewers.

"""Return true if taps aff."""
return self._is_taps_aff

@Throttle(MIN_TIME_BETWEEN_UPDATES)
Copy link
Member

Choose a reason for hiding this comment

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

Not needed when going with SCAN_INTERVAL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


DEFAULT_NAME = 'Taps Aff'

MIN_TIME_BETWEEN_UPDATES = timedelta(minutes=30)
Copy link
Member

Choose a reason for hiding this comment

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

Please use SCAN_INTERVAL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

import (BinarySensorDevice, PLATFORM_SCHEMA)
from homeassistant.const import (CONF_NAME)
import homeassistant.helpers.config_validation as cv
from homeassistant.util import Throttle
Copy link
Member

Choose a reason for hiding this comment

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

Not needed when going with SCAN_INTERVAL.

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've moved to SCAN_INTERVAL will use this for future sensors.

import voluptuous as vol

from homeassistant.components.binary_sensor \
import (BinarySensorDevice, PLATFORM_SCHEMA)
Copy link
Member

Choose a reason for hiding this comment

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

The \ is not needed if the line break is moved.

from homeassistant.components.binary_sensor import (
    BinarySensorDevice, ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

name = config.get(CONF_NAME)
location = config.get(CONF_LOCATION)

taps_aff_data = TapsAffData(location)
Copy link
Member

Choose a reason for hiding this comment

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

I guess that there will be an exception if the location is not available. For users it will be helpful if there is a log entry that indicate this. Make the setup return if the location is not present this will ensure that the users are not having a non-functional platform for their instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An exception is thrown, but I've improved the base library as I found a case where an exception slipped through. I've caught RuntimeException and provide a log message to explain what is wrong.

@fabaff fabaff merged commit a1c119a into home-assistant:dev Jun 4, 2017
@balloob balloob mentioned this pull request Jun 16, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Sep 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants