Skip to content

Improving icloud device tracker#14078

Merged
fabaff merged 5 commits intohome-assistant:devfrom
evgeniy-khatko:dev
May 8, 2018
Merged

Improving icloud device tracker#14078
fabaff merged 5 commits intohome-assistant:devfrom
evgeniy-khatko:dev

Conversation

@evgeniy-khatko
Copy link
Copy Markdown
Contributor

@evgeniy-khatko evgeniy-khatko commented Apr 24, 2018 •
edited
Loading

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.

Sign in to view
@evgeniy-khatko evgeniy-khatko mentioned this pull request Apr 24, 2018
Merged
2 tasks
@evgeniy-khatko
Copy link
Copy Markdown
Contributor Author

evgeniy-khatko commented Apr 25, 2018

Anyone could take a look?

fabaff
fabaff reviewed Apr 28, 2018
View reviewed changes
Comment thread homeassistant/components/device_tracker/icloud.py Outdated
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
Copy Markdown
Member

@fabaff fabaff Apr 28, 2018

Choose a reason for hiding this comment

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

Is not validated -> https://developers.home-assistant.io/docs/en/creating_platform_code_review.html#3-configuration

Copy link
Copy Markdown
Contributor Author

@evgeniy-khatko evgeniy-khatko Apr 28, 2018

Choose a reason for hiding this comment

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

fixed

Copy link
Copy Markdown
Member

@fabaff fabaff Apr 29, 2018

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
Copy Markdown
Contributor Author

@evgeniy-khatko evgeniy-khatko May 1, 2018

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.

fabaff
fabaff reviewed Apr 28, 2018
View reviewed changes
Comment thread homeassistant/components/device_tracker/icloud.py Outdated
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
Copy Markdown
Member

@fabaff fabaff Apr 28, 2018

Choose a reason for hiding this comment

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

Is not validated -> https://developers.home-assistant.io/docs/en/creating_platform_code_review.html#3-configuration

Copy link
Copy Markdown
Contributor Author

@evgeniy-khatko evgeniy-khatko Apr 28, 2018

Choose a reason for hiding this comment

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

fixed

Copy link
Copy Markdown
Member

@fabaff fabaff Apr 29, 2018

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.

Sign in to view
@homeassistant

This comment has been minimized.

Sign in to view
@homeassistant

This comment has been minimized.

Sign in to view
@evgeniy-khatko
Copy link
Copy Markdown
Contributor Author

evgeniy-khatko commented May 8, 2018

Any updates?

fabaff
fabaff approved these changes May 8, 2018
View reviewed changes
Copy link
Copy Markdown
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
Merged
@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.

Reviewers

@fabaff fabaff fabaff approved these changes

Assignees

No one assigned

Labels

cla-signed core integration: device_tracker

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@evgeniy-khatko @homeassistant @fabaff @OlgaKhatko