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 IDTECK proximity card component #18309

Merged
merged 3 commits into from Dec 31, 2018

Conversation

Projects
None yet
5 participants
@dubnom
Copy link
Contributor

dubnom commented Nov 7, 2018

Description:

IDTECK makes proximity card readers. This is a component for interfacing with their products.

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

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.

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

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

  • New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
  • New dependencies are only imported inside functions that use them ([example][ex-import]).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@@ -0,0 +1,92 @@
"""Component for reading from RFK101 proximity card readers.

This comment has been minimized.

@fabaff

fabaff Nov 25, 2018

Member

It's a platform.

"""Component for reading from RFK101 proximity card readers.
For more details about this component, please refer to the documentation at
https://home-assistant.io/components/rfk101/

This comment has been minimized.

@fabaff

fabaff Nov 25, 2018

Member

Should be https://home-assistant.io/components/sensor.idteck_prox/

from homeassistant.components.sensor import PLATFORM_SCHEMA
from homeassistant.const import (
CONF_HOST, CONF_PORT, CONF_NAME, EVENT_HOMEASSISTANT_STOP)
from homeassistant.helpers.entity import Entity

This comment has been minimized.

@fabaff

fabaff Nov 25, 2018

Member

Please keep imports ordered and grouped.


hass.bus.listen_once(EVENT_HOMEASSISTANT_STOP, cleanup)
add_entities([sensor], True)
return True

This comment has been minimized.

@fabaff

fabaff Nov 25, 2018

Member

Not needed.

_LOGGER.error("Could not connect to reader. %s", error)
return False

def cleanup(event):

This comment has been minimized.

@fabaff

fabaff Nov 25, 2018

Member

Docstring is missing.

@property
def device_state_attributes(self):
"""Return supported attributes."""
return {"Host": self._host, "Port": self._port}

This comment has been minimized.

@fabaff

fabaff Nov 25, 2018

Member

Those details are static and coming out of the configuration.

return True


class IDTECKSensor(Entity):

This comment has been minimized.

@fabaff
sensor = IDTECKSensor(host, port, name)
except OSError as error:
_LOGGER.error("Could not connect to reader. %s", error)
return False

This comment has been minimized.

@fabaff

fabaff Nov 25, 2018

Member

Just return.

@property
def state(self):
"""Return the state of the sensor."""
return self._state

This comment has been minimized.

@fabaff

fabaff Nov 25, 2018

Member

Where is that set during the lifetime of the object?


def _callback(self, card):
"""Send a keycard event message into HASS."""
self.hass.bus.fire(

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 29, 2018

Member

Sensors are not supposed to fire custom events. There are already state changed events fired for sensors.

If a custom event is needed, that should be fired from a component. So then create a idteck_prox component that can do that.

@fabaff fabaff changed the title Added IDTECK proximity card sensor component. Added IDTECK proximity card sensor component Dec 12, 2018

@fabaff

This comment has been minimized.

Copy link
Member

fabaff commented Dec 12, 2018

@dubnom, any progress?

@dubnom

This comment has been minimized.

Copy link
Contributor

dubnom commented Dec 12, 2018


return True

class idteck_platform():

This comment has been minimized.

@houndci-bot

houndci-bot Dec 14, 2018

expected 2 blank lines, found 1

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 14, 2018

Member

Please use CapWords convention for class names:
https://www.python.org/dev/peps/pep-0008/#class-names

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 14, 2018

Member

Perhaps IdteckReader?


return True

class idteck_platform():

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 14, 2018

Member

Please use CapWords convention for class names:
https://www.python.org/dev/peps/pep-0008/#class-names

@@ -0,0 +1,70 @@
"""Platform for interfacing RFK101 proximity card readers.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 14, 2018

Member

This is now a component, not a platform.



def setup(hass, config):
"""Set up the IDTECK proximity card platform."""

This comment has been minimized.

@MartinHjelmare
Show resolved Hide resolved homeassistant/components/idteck_prox.py

DOMAIN = "idteck_prox"

EVENT_KEYCARD = 'keycard'

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 14, 2018

Member

Prefix the event string with the component name separated with underscore.


return True

class idteck_platform():

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 14, 2018

Member

Perhaps IdteckReader?

return True

class idteck_platform():
"""Representation of an ITECK proximity card reader."""

This comment has been minimized.

@MartinHjelmare
from rfk101py.rfk101py import rfk101py
self._connection = rfk101py(self._host, self._port, self._callback)

hass.bus.listen_once(EVENT_HOMEASSISTANT_STOP, self.stop)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 14, 2018

Member

Please move this out to setup function. We don't want side effects in init method.

Show resolved Hide resolved homeassistant/components/idteck_prox.py
@dubnom

This comment has been minimized.

Copy link
Contributor

dubnom commented Dec 14, 2018

@MartinHjelmare MartinHjelmare changed the title Added IDTECK proximity card sensor component Add IDTECK proximity card component Dec 14, 2018

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Nice!

Can be merged when build passes.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 31, 2018

Coveralls is flaky.

@MartinHjelmare MartinHjelmare merged commit 4b541f4 into home-assistant:dev Dec 31, 2018

4 of 5 checks passed

coverage/coveralls Coverage decreased (-0.2%) to 92.88%
Details
Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Dec 31, 2018

mxworm added a commit to mxworm/home-assistant that referenced this pull request Jan 3, 2019

Merge branch 'dev' into current
* dev:
  Adds ability to calibrate temperature for BME680 (home-assistant#19684)
  Bumping aioasuswrt version to 1.1.17 (home-assistant#19714)
  Bump pyotgw to 0.4b1 (home-assistant#19715)
  Envisalink pgm (home-assistant#19499)
  Update pyhomematic 0.1.54 + small fixes (home-assistant#19667)
  Add ness alarm control panel using nessclient (home-assistant#18463)
  Add support for color_temp_command_template in MQTT light component (home-assistant#19675)
  Improve rflink coverage (home-assistant#19596)
  Luftdaten traceback (home-assistant#19666)
  Fix london_underground issue (home-assistant#19642)
  Add IDTECK proximity card component (home-assistant#18309)
  Fix homekit_controller pairing regression (home-assistant#19654)
  Fix error in got_connected for remote.harmony (home-assistant#19662)
  Fix exception checking for next dublin bus (home-assistant#19663)
  Added regexp validation allowing Twilio notifications to use Sender ID instead of phone number (home-assistant#19644)
  pytraccar version bump (home-assistant#19659)
  Suppress traceback if network is not available

@balloob balloob referenced this pull request Jan 10, 2019

Merged

0.85.0 #19897

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment