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

BT Home Hub 5 device tracker support #2250

Merged
merged 1 commit into from
Jun 15, 2016
Merged

BT Home Hub 5 device tracker support #2250

merged 1 commit into from
Jun 15, 2016

Conversation

lwis
Copy link
Member

@lwis lwis commented Jun 6, 2016

Description:

Support for the BT Home Hub 5 for tracking devices.

Looking for feedback as this is my first component addition to the project, I'll add the documentation PR before (if) this is merged.

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

Example entry for configuration.yaml (if applicable):

device_tracker:
  platform: bt_home_hub_5
  host: 192.168.1.254
  interval_seconds: 10
  consider_home: 180
  track_new_devices: yes

Checklist:

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

If code communicates with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass

dirty_json = root.find('known_device_list').get('value')

# Normalise the JavaScript data to JSON.
clean_json = unquote(dirty_json.replace('\'', '\"').replace('{', '{\"').replace(':\"', '\":\"').replace('\",', '\",\"'))

Choose a reason for hiding this comment

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

farcy v1.1

  • 80: E501 line too long (128 > 79 characters)

@balloob
Copy link
Member

balloob commented Jun 9, 2016

The code seems fine. You will need to clean it up to adhere to linting standards. See the instructions above in the PR message.

@balloob
Copy link
Member

balloob commented Jun 12, 2016

Please follow the checklist in your own PR message. Still some steps left.

@lwis
Copy link
Member Author

lwis commented Jun 13, 2016

@balloob home-assistant/home-assistant.io#558 - all should be in order 🎱

@balloob
Copy link
Member

balloob commented Jun 13, 2016

Please add your file to .coveragerc

@lwis
Copy link
Member Author

lwis commented Jun 13, 2016

@balloob why would I need to add to the omissions? I've added a test, and coveralls is passing.

@balloob
Copy link
Member

balloob commented Jun 13, 2016

New components are not allowed to reduce test coverage or else they should
be added to coveragerc. Your test is pretty bogus anyway as a component
with the following code would pass…

def get_scanner(hass, config):
return 1

On Mon, Jun 13, 2016 at 9:21 AM, Lewis Juggins [email protected]
wrote:

@balloob https://github.com/balloob why would I need to the omissions?
I've added a test, and coveralls is passing.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2250 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABYJ2s8ZhYeJ2UkhiNEVayWDUYgwf0Rlks5qLYN-gaJpZM4IvGBm
.

PaulusSchoutsen.nl
It's nice to be important but it's more important to be nice.

@lwis
Copy link
Member Author

lwis commented Jun 13, 2016

Fair enough, squashed and omitted the component. When I get some time I'll improve test coverage for the component.

@balloob
Copy link
Member

balloob commented Jun 15, 2016

Alrighty, looks bueno 🐬 🚀

@balloob balloob merged commit 7b8b78e into home-assistant:dev Jun 15, 2016
@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants