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 dovado to a component and sensor platform #20339

Merged
merged 7 commits into from Jan 27, 2019

Conversation

@rohankapoorcom
Copy link
Member

rohankapoorcom commented Jan 23, 2019

Description:

Per home-assistant/architecture#131, sensor platforms should not be registering services. Dovado is one of several that does and this should be cleaned up. I moved all of the setup/service logic to a new Dovado component and then used that component inside the sensor platform.

I am unable to verify that any of these changes work since I do not have a Dovado router.

Breaking Change: The Dovado sensor platform has been broken up into a Dovado component with sensor and notify platforms. The configuration variables have been changed. Please refer to the documentation for the correct configuration.

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

Example entry for configuration.yaml (if applicable):

dovado:
  username: YOUR_USERNAME
  password: YOUR_PASSWORD

sensor:
  - platform: dovado
    sensors:
      - network

notify:
 - platform: dovado

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.
Show resolved Hide resolved homeassistant/components/dovado/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/dovado/sensor.py Outdated
Show resolved Hide resolved homeassistant/components/dovado/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/dovado/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/dovado/__init__.py
Show resolved Hide resolved homeassistant/components/dovado/sensor.py Outdated
Show resolved Hide resolved homeassistant/components/dovado/sensor.py Outdated
Show resolved Hide resolved homeassistant/components/dovado/sensor.py Outdated

rohankapoorcom added a commit to rohankapoorcom/home-assistant that referenced this pull request Jan 26, 2019

rohankapoorcom added a commit to rohankapoorcom/home-assistant that referenced this pull request Jan 26, 2019

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Looks good!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 26, 2019

I think we can merge when build passes and we've added a paragraph in the PR description about the breaking changes for the release notes.

@rohankapoorcom

This comment has been minimized.

Copy link
Member Author

rohankapoorcom commented Jan 26, 2019

I guess its time to go update the docs as well.

@rohankapoorcom

This comment has been minimized.

Copy link
Member Author

rohankapoorcom commented Jan 26, 2019

Docs linked, build seems to be failing due to an unrelated file have a lint error.

rohankapoorcom added some commits Jan 23, 2019

@rohankapoorcom rohankapoorcom force-pushed the rohankapoorcom:dovado-split branch from a3d518d to 18cfa1a Jan 27, 2019

@rohankapoorcom

This comment has been minimized.

Copy link
Member Author

rohankapoorcom commented Jan 27, 2019

Rebasing with dev to get the fix for the linting error.

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Great!

@MartinHjelmare MartinHjelmare merged commit 7368c62 into home-assistant:dev Jan 27, 2019

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.002%) to 92.989%
Details

@wafflebot wafflebot bot removed the in progress label Jan 27, 2019

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 27, 2019

For the future maybe we should move configuration to the component and load the platforms via discovery instead? But it's not required I think.

mxworm added a commit to mxworm/home-assistant that referenced this pull request Jan 27, 2019

Split out dovado to a component and sensor platform (home-assistant#2…
…0339)

* Split out dovado to a component and sensor platform

* Lint

* Address code review comments (home-assistant#20339)

* Switch to using a notify platform for dovado SMS (home-assistant#20339)

* Optimizing imports

* Remove return on `setup_platform`.

* Clean up unneeded constants

fredrike added a commit to fredrike/home-assistant that referenced this pull request Jan 30, 2019

Split out dovado to a component and sensor platform (home-assistant#2…
…0339)

* Split out dovado to a component and sensor platform

* Lint

* Address code review comments (home-assistant#20339)

* Switch to using a notify platform for dovado SMS (home-assistant#20339)

* Optimizing imports

* Remove return on `setup_platform`.

* Clean up unneeded constants

@balloob balloob referenced this pull request Feb 6, 2019

Merged

0.87.0 #20794

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment