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

Async version of melissa #17721

Merged
merged 3 commits into from Oct 30, 2018

Conversation

Projects
None yet
6 participants
@kennedyshead
Contributor

kennedyshead commented Oct 23, 2018

Description:

This will make Melissa component async

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
@@ -34,10 +34,10 @@
]
def setup_platform(hass, config, add_entities, discovery_info=None):
async def async_setup_platform(hass, config, add_entities, discovery_info=None):

This comment has been minimized.

@houndci-bot

houndci-bot Oct 23, 2018

line too long (80 > 79 characters)

@kennedyshead kennedyshead changed the title from Async version of melissa to WIP: Async version of melissa Oct 23, 2018

@kennedyshead kennedyshead changed the title from WIP: Async version of melissa to Async version of melissa Oct 23, 2018

@kennedyshead kennedyshead force-pushed the kennedyshead:melissa_async branch 2 times, most recently from 2ac7d5a to b3234c4 Oct 23, 2018

@kennedyshead kennedyshead force-pushed the kennedyshead:melissa_async branch from b3234c4 to a41217a Oct 25, 2018

self.hum.update()
assert self.hum.state is None
await self.hum.async_device_update()
assert self.hum.state is None

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2018

blank line at end of file

@@ -255,4 +264,4 @@ def test_hass_fan_to_melissa(self, mocked_warning):
assert 3 == self.thermostat.hass_fan_to_melissa(SPEED_HIGH)
self.thermostat.hass_fan_to_melissa("test")
mocked_warning.assert_called_once_with(
"Melissa have no setting for %s fan mode", "test")
"Melissa have no setting for %s fan mode", "test")

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2018

blank line at end of file

@kennedyshead kennedyshead force-pushed the kennedyshead:melissa_async branch from a41217a to 9f409ae Oct 25, 2018

hass.data[DATA_MELISSA] = api
load_platform(hass, 'sensor', DOMAIN, {})
load_platform(hass, 'climate', DOMAIN, {})
await async_load_platform(hass, 'sensor', DOMAIN, {})

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 25, 2018

Member

Don't await these but create tasks instead. Use hass.async_create_task. Otherwise there could be a deadlock where the platform is waiting for the component which is waiting for the platform...

@@ -54,9 +55,10 @@ def state(self):
"""Return the state of the sensor."""
return self._state
def update(self):
async def async_device_update(self, warning=False):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 25, 2018

Member

We should not overwrite this method. Define async def async_update.

"""Fetch status from melissa."""
self._data = self._api.status(cached=True)
await super(MelissaSensor, self).async_device_update(warning)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 25, 2018

Member

Remove this.

@@ -70,9 +72,9 @@ def unit_of_measurement(self):
"""Return the unit of measurement."""
return self._unit
def update(self):
async def async_device_update(self, warning=False):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 25, 2018

Member

See above.

@@ -90,9 +92,9 @@ def unit_of_measurement(self):
"""Return the unit of measurement."""
return self._unit
def update(self):
async def async_device_update(self, warning=False):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 25, 2018

Member

See above.

@kennedyshead kennedyshead force-pushed the kennedyshead:melissa_async branch from 9f409ae to 1e0373f Oct 26, 2018

@kennedyshead

This comment has been minimized.

Contributor

kennedyshead commented Oct 26, 2018

@MartinHjelmare thank you so much for the review :)

load_platform(hass, 'sensor', DOMAIN, {})
load_platform(hass, 'climate', DOMAIN, {})
hass.async_create_task(async_load_platform(hass, 'sensor', DOMAIN, {}))
await async_load_platform(hass, 'climate', DOMAIN, {})

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 26, 2018

Member

Change this line too as the one above.

"""Fetch new state data for the sensor."""
super().update()
await super().async_device_update(warning)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 26, 2018

Member

Replace this too.

"""Fetch new state data for the sensor."""
super().update()
await super().async_device_update(warning)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 26, 2018

Member

See above.

@kennedyshead kennedyshead force-pushed the kennedyshead:melissa_async branch from 1e0373f to 829b4b4 Oct 26, 2018

@kennedyshead

This comment has been minimized.

Contributor

kennedyshead commented Oct 26, 2018

Could async_device_update be internal instead maybe? Thank you so much again :)

@@ -54,9 +55,9 @@ def state(self):
"""Return the state of the sensor."""
return self._state
def update(self):
async def async_update(self, warning=False):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 26, 2018

Member

There's no warning parameter in this signature.

@@ -70,9 +71,9 @@ def unit_of_measurement(self):
"""Return the unit of measurement."""
return self._unit
def update(self):
async def async_update(self, warning=False):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 26, 2018

Member

See above.

"""Fetch new state data for the sensor."""
super().update()
await super().async_update(warning)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 26, 2018

Member

See above.

@@ -90,9 +91,9 @@ def unit_of_measurement(self):
"""Return the unit of measurement."""
return self._unit
def update(self):
async def async_update(self, warning=False):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 26, 2018

Member

See above.

"""Fetch new state data for the sensor."""
super().update()
await super().async_update(warning)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 26, 2018

Member

See above.

@kennedyshead kennedyshead force-pushed the kennedyshead:melissa_async branch from 829b4b4 to 9f37765 Oct 26, 2018

@kennedyshead

This comment has been minimized.

Contributor

kennedyshead commented Oct 26, 2018

@MartinHjelmare So sorry bout that! Took you longer to review than me to fix an obvious error :/

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 26, 2018

No worries! I agree we could make it more clear what entity methods are supposed to be used outside core and helpers.

@kennedyshead

This comment has been minimized.

Contributor

kennedyshead commented Oct 26, 2018

I think it just should be replaced with _async_device_update

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 26, 2018

The reason it isn't private is that we're calling it from the entity platform helper, ie outside the entity class.

@kennedyshead

This comment has been minimized.

Contributor

kennedyshead commented Oct 26, 2018

Ok... So designflaw :P ;)

@kennedyshead kennedyshead force-pushed the kennedyshead:melissa_async branch from 9f37765 to 8f86ee7 Oct 27, 2018

@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@@ -34,10 +34,11 @@
]
def setup_platform(hass, config, add_entities, discovery_info=None):
async def async_setup_platform(
hass, config, add_entities, discovery_info=None):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 27, 2018

Member

Please rename add_entities to async_add_entities.

@@ -15,11 +15,12 @@
_LOGGER = logging.getLogger(__name__)
def setup_platform(hass, config, add_entities, discovery_info=None):
async def async_setup_platform(
hass, config, add_entities, discovery_info=None):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 27, 2018

Member

See above.

self.thermostat = melissa.MelissaClimate(
self.api, device['serial_number'], device)
self.thermostat.update()
def tearDown(self): # pylint: disable=invalid-name
"""Teardown this test class. Stop hass."""
self.hass.stop()
@patch("homeassistant.components.climate.melissa.MelissaClimate")

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 27, 2018

Member

We can't use decorator to patch coroutine tests. Please either use pytest fixture or patch inside test with context manager.

def tearDown(self): # pylint: disable=invalid-name
"""Teardown this test class. Stop hass."""
self.hass.stop()
@patch("homeassistant.components.climate.melissa.MelissaClimate")
def test_setup_platform(self, mocked_thermostat):
async def test_setup_platform(self, mocked_thermostat):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 27, 2018

Member

Async tests need to be rewritten as standalone pytest functions.

@@ -37,15 +37,16 @@ def tearDown(self): # pylint: disable=invalid-name
"""Teardown this test class. Stop hass."""
self.hass.stop()
def test_setup_platform(self):
async def test_setup_platform(self):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 27, 2018

Member

See above.

@@ -25,12 +25,13 @@ def tearDown(self): # pylint: disable=invalid-name
self.hass.stop()
@MockDependency("melissa")
def test_setup(self, mocked_melissa):
async def test_setup(self, mocked_melissa):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 27, 2018

Member

See above.

@kennedyshead kennedyshead force-pushed the kennedyshead:melissa_async branch from 8f86ee7 to 13a5c3c Oct 27, 2018

@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 27, 2018

Please don't squash commits after review has started. Squashing makes it hard to follow the changes. Thanks!

@kennedyshead

This comment has been minimized.

Contributor

kennedyshead commented Oct 27, 2018

@MartinHjelmare Sorry, old habit. I'll squash when its finished from now on.

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 27, 2018

You don't need to squash. We'll squash upon merge.

@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@kennedyshead

This comment has been minimized.

Contributor

kennedyshead commented Oct 30, 2018

@MartinHjelmare Where there anything more that needs fixing? I need this PR to integrate a new water-heater device ;)

MagnusKnutas added some commits Oct 30, 2018

@kennedyshead kennedyshead force-pushed the kennedyshead:melissa_async branch from 84575bd to eb33786 Oct 30, 2018

@MartinHjelmare MartinHjelmare merged commit 4073f63 into home-assistant:dev Oct 30, 2018

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.007%) to 93.008%
Details

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

@kennedyshead kennedyshead deleted the kennedyshead:melissa_async branch Oct 31, 2018

@balloob balloob referenced this pull request Nov 9, 2018

Merged

0.82 #18335

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