Hydroquebec component use now asyncio by titilambert · Pull Request #10795 · home-assistant/core

titilambert · 2017-11-25T16:31:55Z

Description:

Hydroquebec component use now asyncio
This also update pyhydroquebec version

Checklist:

If user exposed functionality or configuration variables are added/changed:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

houndci-bot · 2017-11-25T16:32:05Z

'time' imported but unused

MartinHjelmare

Two minor comments. Otherwise looks good.

MartinHjelmare · 2017-11-26T23:40:14Z

Rename add_devices to async_add_devices.

MartinHjelmare · 2017-11-26T23:40:26Z

Same as above.

houndci-bot · 2017-11-28T00:23:32Z

undefined name 'aasync_add_devices'

this error makes me wonder if you tested your code? 🤔

I checked the box on the first commit, before updating this line...
But as always, quick patch, bad patch ...

balloob · 2017-11-28T07:23:26Z

This needs to be an async_update

@balloob This is not the update method directly call by hass, this method is called by an other async_update method.
So my question is: do coroutine names have to start by async_ ?

Good point! No they don't. Only the entity method needs to be named async_update for the code to work, if it's a coroutine. But for clarity it could be wise to have this method be called async_update too.

houndci-bot · 2017-11-30T06:20:45Z

expected 2 blank lines, found 1

houndci-bot · 2017-11-30T06:20:45Z

'tempfile' imported but unused

andrey-git · 2017-11-30T06:22:38Z

If you add tests please remove homeassistant/components/sensor/hydroquebec.py from coverageac

titilambert · 2017-11-30T06:26:04Z

@andrey-git thanks ! It's my first tests in HASS ...

houndci-bot · 2017-11-30T14:06:34Z

line too long (81 > 79 characters)

MartinHjelmare · 2017-12-12T09:02:39Z

+    def fetch_data(self):
+        """Return fake fetching data."""
+        if hasattr(self, 'data_fetched'):
+            raise PyHydroQuebecErrorMock("Fake Error")


Don't do this here. Just patch the call that should error, and add a side effect that raises the error. Tests should also be totally isolated from each other.

MartinHjelmare · 2017-12-12T09:03:52Z

 pytest-timeout>=1.2.0
 pytest-catchlog>=1.2.2
 pytest-sugar>=0.7.1
+pytest-asyncio>=0.6.0


I don't think you need new test requirements.

MartinHjelmare · 2017-12-12T09:04:55Z

+            ],
+        }
+    }
+    ret = yield from hydroquebec.async_setup_platform(hass, config,


Since you set up the component below, you should not set up the platform here.

MartinHjelmare · 2017-12-12T09:06:10Z

+
+
+@asyncio.coroutine
+def test_hydroquebec_sensor_2(hass):


Name the test functions according to what they test. You don't need to include the module name in all of the test names, since that is assumed already.

houndci-bot · 2017-12-15T03:34:25Z

expected 2 blank lines, found 1

houndci-bot · 2017-12-15T03:34:25Z

expected 1 blank line, found 0

houndci-bot · 2017-12-15T03:34:25Z

'pytest' imported but unused

titilambert · 2017-12-15T06:02:43Z

Ready for review, I hope this is the good one !

MartinHjelmare · 2017-12-16T20:47:51Z

-    except requests.exceptions.HTTPError as error:
-        _LOGGER.error("Failt login: %s", error)
+                     ", ".join(contracts))
+    except BaseException as error:


This can't happen here, since you already catch this error in HydroquebecData._fetch_data.

houndci-bot · 2017-12-17T16:04:09Z

local variable 'ret' is assigned to but never used

houndci-bot · 2017-12-17T16:04:09Z

local variable 'ret' is assigned to but never used

MartinHjelmare · 2017-12-17T16:07:45Z

This isn't used anymore I think?

MartinHjelmare · 2017-12-17T16:09:19Z

You could subclass HydroQuebecClientMock, instead of writing the whole class again.

titilambert · 2017-12-17T17:47:37Z

@MartinHjelmare changes done

MartinHjelmare · 2017-12-17T19:49:54Z

This test is missing an assertion. I think you should pass caplog here and assert that the text contains "Error on receive last Hydroquebec data...". With that this should be good to go.

MartinHjelmare

Thanks! Good work! 🎉

titilambert requested a review from andrey-git as a code owner November 25, 2017 16:31

homeassistant added cla-signed platform: sensor.hydroquebec labels Nov 25, 2017

houndci-bot reviewed Nov 25, 2017

View reviewed changes

Comment thread homeassistant/components/sensor/hydroquebec.py Outdated

Copy link
Copy Markdown

houndci-bot Nov 25, 2017

Choose a reason for hiding this comment

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

'time' imported but unused

titilambert force-pushed the hydroquebec branch from 91d3be6 to 570f0b6 Compare November 25, 2017 16:32

homeassistant added the cla-signed label Nov 25, 2017

balloobbot added platform requirement labels Nov 25, 2017

titilambert force-pushed the hydroquebec branch from 570f0b6 to 6352093 Compare November 25, 2017 16:46

homeassistant added the cla-signed label Nov 25, 2017

MartinHjelmare reviewed Nov 26, 2017

View reviewed changes

titilambert force-pushed the hydroquebec branch from 6352093 to 1889034 Compare November 28, 2017 00:23

homeassistant added the cla-signed label Nov 28, 2017

houndci-bot reviewed Nov 28, 2017

View reviewed changes

balloob reviewed Nov 28, 2017

View reviewed changes

titilambert force-pushed the hydroquebec branch from 1889034 to 4f5198f Compare November 28, 2017 14:41

homeassistant added the cla-signed label Nov 28, 2017

titilambert force-pushed the hydroquebec branch from 4f5198f to 29d0b57 Compare November 29, 2017 05:10

homeassistant added the cla-signed label Nov 29, 2017

houndci-bot reviewed Nov 30, 2017

View reviewed changes

titilambert force-pushed the hydroquebec branch from 25f2663 to 6199663 Compare November 30, 2017 06:21

homeassistant added the cla-signed label Nov 30, 2017

titilambert force-pushed the hydroquebec branch from 6199663 to a43ec72 Compare November 30, 2017 06:24

homeassistant added the cla-signed label Nov 30, 2017

titilambert force-pushed the hydroquebec branch from a43ec72 to 8a7ee36 Compare November 30, 2017 14:06

homeassistant added the cla-signed label Nov 30, 2017

houndci-bot reviewed Nov 30, 2017

View reviewed changes

Comment thread tests/components/sensor/test_hydroquebec.py Outdated

Copy link
Copy Markdown

houndci-bot Nov 30, 2017

Choose a reason for hiding this comment

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

line too long (81 > 79 characters)

titilambert force-pushed the hydroquebec branch from 8a7ee36 to c999f40 Compare December 11, 2017 01:44

homeassistant added the cla-signed label Dec 12, 2017

MartinHjelmare requested changes Dec 12, 2017

View reviewed changes

titilambert mentioned this pull request Dec 12, 2017

Could not get annual data titilambert/pyhydroquebec#5

Closed

houndci-bot reviewed Dec 15, 2017

View reviewed changes

titilambert force-pushed the hydroquebec branch from 70f849f to 3041dbe Compare December 15, 2017 03:35

homeassistant added the cla-signed label Dec 15, 2017

fix tests

77c4f43

titilambert force-pushed the hydroquebec branch from 3041dbe to 77c4f43 Compare December 15, 2017 05:13

homeassistant added the cla-signed label Dec 15, 2017

MartinHjelmare requested changes Dec 17, 2017

View reviewed changes

houndci-bot reviewed Dec 17, 2017

View reviewed changes

Comment thread homeassistant/components/sensor/hydroquebec.py Outdated

Copy link
Copy Markdown

houndci-bot Dec 17, 2017

Choose a reason for hiding this comment

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

local variable 'ret' is assigned to but never used

houndci-bot reviewed Dec 17, 2017

View reviewed changes

Comment thread homeassistant/components/sensor/hydroquebec.py Outdated

Copy link
Copy Markdown

houndci-bot Dec 17, 2017

Choose a reason for hiding this comment

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

local variable 'ret' is assigned to but never used

titilambert force-pushed the hydroquebec branch from 8baa0da to aa9dcb2 Compare December 17, 2017 16:05

homeassistant added the cla-signed label Dec 17, 2017

MartinHjelmare reviewed Dec 17, 2017

View reviewed changes

titilambert force-pushed the hydroquebec branch from aa9dcb2 to 9051ac8 Compare December 17, 2017 16:43

homeassistant added the cla-signed label Dec 17, 2017

MartinHjelmare reviewed Dec 17, 2017

View reviewed changes

Remove useless try/except and associated tests

045337e

titilambert force-pushed the hydroquebec branch from 9051ac8 to 045337e Compare December 17, 2017 20:11

homeassistant added the cla-signed label Dec 17, 2017

MartinHjelmare approved these changes Dec 17, 2017

View reviewed changes

MartinHjelmare merged commit 8742ce0 into home-assistant:dev Dec 17, 2017

balloob mentioned this pull request Jan 11, 2018

0.61 #11589

Merged

home-assistant locked and limited conversation to collaborators Mar 30, 2018

ghost added integration: hydroquebec and removed platform: sensor.hydroquebec labels Mar 21, 2019

Uh oh!

Conversation

titilambert commented Nov 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrey-git commented Nov 30, 2017

Uh oh!

titilambert commented Nov 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

titilambert commented Dec 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

titilambert commented Dec 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

titilambert commented Nov 25, 2017 •

edited

Loading