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

Tibber component and notify #17062

Merged
merged 9 commits into from Oct 4, 2018

Conversation

@Danielhiversen
Copy link
Member

commented Oct 2, 2018

Description:

Move Tibber to a separate component.
Make a Tibber notify service

Breaking change: Tibber can not be configured as a sensor anymore.

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for configuration.yaml (if applicable):

tibber:
  access_token: d11a43897efa4cf478afd659d6c8b7117da9e33b38232fd454b0e9f28af98012

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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.

Danielhiversen added some commits Oct 1, 2018

@MartinHjelmare
Copy link
Member

left a comment

Looks good! Some comments.


from homeassistant.components.notify import (
ATTR_TITLE, ATTR_TITLE_DEFAULT, BaseNotificationService)
from homeassistant.components.tibber import DOMAIN

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

Please import the tibber domain as another name. This platform belongs to the notify domain.

try:
await self._notify(title=title, message=message)
except asyncio.TimeoutError:
_LOGGER.error("Timeout sending message with Tibber.")

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

Please don't end logging messages with period.



def get_service(hass, config, discovery_info=None):
"""Get the Pushbullet notification service."""

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

Stale docstring.

@@ -0,0 +1,37 @@
"""
Pushbullet platform for notify component.

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

Stale docstring.

import homeassistant.helpers.config_validation as cv
from homeassistant.components.sensor import PLATFORM_SCHEMA
from homeassistant.const import EVENT_HOMEASSISTANT_STOP, CONF_ACCESS_TOKEN
from homeassistant.components.tibber import DOMAIN

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

See above.

await tibber_connection.rt_disconnect()
hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, _close)
tibber_connection = hass.data.get(DOMAIN)
if tibber_connection is None:

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

Make a guard clause above instead and check if discovery_info is None and return if so.


for component in ['sensor', 'notify']:
discovery.load_platform(hass, component, DOMAIN,
{CONF_NAME: DOMAIN}, conf)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

The last argument should be the original complete config.

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

When is the name and domain discovery_info used?

I remembered, it's in the notify component to name the platform.

This comment has been minimized.

Copy link
@Danielhiversen

Danielhiversen Oct 2, 2018

Author Member

Yes, that is right.

(Thanks for another good review)

_LOGGER = logging.getLogger(__name__)


def get_service(hass, config, discovery_info=None):

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

I'm thinking we can make this a coroutine and call it async_get_service.

@MartinHjelmare
Copy link
Member

left a comment

Nice!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

Can be merged when build passes and docs PR is linked.

websession=async_get_clientsession(hass))
hass.data[DOMAIN] = tibber_connection

async def _close(*_):

This comment has been minimized.

Copy link
@pvizeli

pvizeli Oct 3, 2018

Member

that is simple _close(event)

@pvizeli pvizeli merged commit 05d8c57 into dev Oct 4, 2018

6 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
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 93.582%
Details

@wafflebot wafflebot bot removed the in progress label Oct 4, 2018

@pvizeli pvizeli deleted the tibber_component branch Oct 4, 2018

@pvizeli

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

👍

@Danielhiversen

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

I am not finished with the documentation, but hope to fix that soon.

@Danielhiversen Danielhiversen referenced this pull request Oct 4, 2018

Merged

Tibber component #6504

0 of 2 tasks complete
@pvizeli

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

I don't worry that you don't do it

@balloob balloob referenced this pull request Oct 12, 2018

Merged

0.80.0 #17361

@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.