Skip to content

Split out dovado to a component and sensor platform#20339

Merged
MartinHjelmare merged 7 commits into
home-assistant:devfrom
rohankapoorcom:dovado-split
Jan 27, 2019
Merged

Split out dovado to a component and sensor platform#20339
MartinHjelmare merged 7 commits into
home-assistant:devfrom
rohankapoorcom:dovado-split

Conversation

@rohankapoorcom
Copy link
Copy Markdown
Member

@rohankapoorcom 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.

Comment thread homeassistant/components/dovado/__init__.py Outdated
Comment thread homeassistant/components/dovado/__init__.py Outdated
Comment thread homeassistant/components/dovado/__init__.py Outdated
Comment thread homeassistant/components/dovado/sensor.py Outdated
Comment thread homeassistant/components/dovado/__init__.py Outdated
Comment thread homeassistant/components/dovado/__init__.py Outdated
Comment thread homeassistant/components/dovado/__init__.py Outdated
Comment thread homeassistant/components/dovado/sensor.py Outdated
Comment thread homeassistant/components/dovado/sensor.py Outdated
Comment thread homeassistant/components/dovado/sensor.py Outdated
Comment thread homeassistant/components/dovado/__init__.py Outdated
Comment thread homeassistant/components/dovado/__init__.py Outdated
Copy link
Copy Markdown
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!

@MartinHjelmare
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Member Author

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

@rohankapoorcom
Copy link
Copy Markdown
Member Author

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

@rohankapoorcom
Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
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.

Great!

@MartinHjelmare MartinHjelmare merged commit 7368c62 into home-assistant:dev Jan 27, 2019
@ghost ghost removed the in progress label Jan 27, 2019
@MartinHjelmare
Copy link
Copy Markdown
Member

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.

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.

5 participants