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

Split out gpslogger into a separate component and platform #20044

Merged
merged 4 commits into from Jan 14, 2019

Conversation

rohankapoorcom
Copy link
Member

@rohankapoorcom rohankapoorcom commented Jan 13, 2019

Description:

Splits the gpslogger device tracker to be a component in addition to the device_tracker platform. This is required to migrate it over to use the webhook component.

Breaking Change: The gpslogger device_tracker platform no longer takes any configuration. The configuration needs to be applied to the component gpslogger instead. The platform will be automatically loaded with the component and should not be specified in configuration.yaml.

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

gpslogger:

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).
  • New dependencies are only imported inside functions that use them (example).
  • 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.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

We should update coveragerc too. What's the coverage? Can we remove the device tracker platform or should we add the component?

@rohankapoorcom
Copy link
Member Author

rohankapoorcom commented Jan 13, 2019

The device tracker platform is currently in .coveragerc. The component is at 83.33%

I can add a couple more component tests and I think we can remove the device tracker platform from .coveragerc.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Nice!

@MartinHjelmare
Copy link
Member

Can be merged when build passes.

@rohankapoorcom
Copy link
Member Author

Build seems to have failed due to unrelated things, will close/reopen to trigger another.

@ghost ghost removed the in progress label Jan 13, 2019
@ghost ghost added the in progress label Jan 13, 2019
@MartinHjelmare
Copy link
Member

If it's a known flaky test we can just ignore it. But let's see what this build has to offer.

@rohankapoorcom
Copy link
Member Author

rohankapoorcom commented Jan 13, 2019

Looks like the build succeeded this time. We have 100% coverage on both the component and the platform.

@MartinHjelmare MartinHjelmare merged commit 7f38710 into home-assistant:dev Jan 14, 2019
@ghost ghost removed the in progress label Jan 14, 2019
@rohankapoorcom rohankapoorcom deleted the gpslogger-component branch January 14, 2019 00:19
@balloob balloob mentioned this pull request Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants