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

Improving icloud device tracker #14078

Merged
merged 5 commits into from May 8, 2018
Merged

Conversation

evgeniy-khatko
Copy link
Contributor

@evgeniy-khatko evgeniy-khatko commented Apr 24, 2018

Description:

  1. Adds ability to choose gps_accuracy for icloud updates
  2. Allows to set custom max refresh interval
  3. Fixes bug where device_tracker.icloud_update did not actually updated the device_tracker state

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

device_tracker:
  - platform: icloud
    username: email@email.com
    password: !secret pwd
    max_interval: 10
    gps_accuracy_threshold: 1000

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
    Before commit
Results (305.34s):
    3553 passed
     100 skipped

After commit

Results (314.74s):
    3553 passed
     100 skipped
$pylint homeassistant/components/device_tracker/icloud.py 
Using config file /Volumes/work/home/home-assistant/pylintrc

-------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 8.97/10, +1.03)

$ pydocstyle homeassistant/components/device_tracker/icloud.py 
$ flake8 homeassistant/components/device_tracker/icloud.py 
$

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).
  • 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.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
    Component does not have tests yet. Might be a matter for next commit.

@homeassistant

This comment has been minimized.

@evgeniy-khatko
Copy link
Contributor Author

Anyone could take a look?

@@ -79,8 +80,11 @@ def setup_scanner(hass, config: dict, see, discovery_info=None):
username = config.get(CONF_USERNAME)
password = config.get(CONF_PASSWORD)
account = config.get(CONF_ACCOUNTNAME, slugify(username.partition('@')[0]))
max_interval = config.get(CONF_MAX_INTERVAL, 30)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Default should be set in the PLATFORM_SCHEMA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, fixed that. Re-ran all tests and checks. Tested manually that defaults values do take effect.

@@ -79,8 +80,11 @@ def setup_scanner(hass, config: dict, see, discovery_info=None):
username = config.get(CONF_USERNAME)
password = config.get(CONF_PASSWORD)
account = config.get(CONF_ACCOUNTNAME, slugify(username.partition('@')[0]))
max_interval = config.get(CONF_MAX_INTERVAL, 30)
gps_accuracy_threshold = config.get(CONF_GPS_ACCURACY_THRESHOLD, 1000)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Dito

@homeassistant

This comment has been minimized.

@homeassistant

This comment has been minimized.

@homeassistant

This comment has been minimized.

@evgeniy-khatko
Copy link
Contributor Author

Any updates?

Copy link
Member

@fabaff fabaff left a comment

Choose a reason for hiding this comment

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

Thanks 🐦

@fabaff fabaff merged commit 9c7523d into home-assistant:dev May 8, 2018
@balloob balloob mentioned this pull request May 28, 2018
girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018
* Improving icloud device tracker

* Adding config validations for new values

* Adding config validations for new values

* Moving icloud specific setup to platform schema. Setting default in platform schema.
@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 2018
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

4 participants