Skip to content

Restore for device_tracker#6150

Merged
kellerza merged 2 commits into
home-assistant:devfrom
kellerza:restore_dt
Feb 22, 2017
Merged

Restore for device_tracker#6150
kellerza merged 2 commits into
home-assistant:devfrom
kellerza:restore_dt

Conversation

@kellerza
Copy link
Copy Markdown
Member

Description:
Add restore (#4614) to device_tracker.

Checklist:

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@mention-bot
Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably want a longitude here :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Indeed, thanks :-)

I still need to do tests that is why I marked it wip. Would prefer to have a helper from input_select

@emlove
Copy link
Copy Markdown
Contributor

emlove commented Feb 21, 2017

Well, I was just about to start looking at this. Glad I happened to look at PRs first!

It looks reasonable to me. I think it goes without saying it would be nice if we could use EntityComponent within device tracker to eliminate some of the duplication, but I'm guessing that wasn't done because the differences are too great.

@kellerza
Copy link
Copy Markdown
Member Author

Migrating this to EntityComponent could open up a whole new can of worms, so lets rather keep for another PR: One of the unnecessary things would be scan_interval that I could think of. Here we also use "see" and only one class of device.

For now the async_added_to_hass is the only duplication introduced here, so should be ok

@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 22, 2017

Another problem with the EntityComponent is that DeviceTracker supports two types of platforms. The router ones that just provide a "get_devices" method and the ones that just call "see"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it should be state.attributes, not state.attr (I fixed this in your original PR too 🕺 )

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, I wasn't too sure :-)

Will still add tests and remove the WIP tag

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Heads up, I will be cutting the release branch tonight.

@kellerza kellerza changed the title WIP: Restore for device_tracker Restore for device_tracker Feb 22, 2017
@kellerza kellerza merged commit 74837db into home-assistant:dev Feb 22, 2017
@kellerza kellerza deleted the restore_dt branch February 22, 2017 20:55
@kellerza kellerza mentioned this pull request Feb 26, 2017
10 tasks
@home-assistant home-assistant locked and limited conversation to collaborators Jun 2, 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.

5 participants