Fix niko_home_control integration by NoUseFreak · Pull Request #23093 · home-assistant/core

NoUseFreak · 2019-04-14T10:13:59Z

Breaking Change:

No breaking changes

Description:

Update base library and only pull for state once.

Related issue: fixes #20005 and #19283

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

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:

~~Documentation added/updated in home-assistant.io~~

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly (example).
New dependencies have been added to requirements in the manifest (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.~~

If the code does not interact with devices:

~~Tests have been added to verify that the new code works.~~

NoUseFreak · 2019-04-14T11:47:38Z

Can I get some assistance in getting the build green :-)

Also, just want to make sure, the package I depend on has it's own dependencies, should these be listed in the manifests?

codecov · 2019-04-14T11:48:32Z

Codecov Report

Merging #23093 into dev will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev   #23093      +/-   ##
==========================================
+ Coverage   93.95%   93.97%   +0.01%     
==========================================
  Files         452      450       -2     
  Lines       36788    36878      +90     
==========================================
+ Hits        34566    34656      +90     
  Misses       2222     2222

Impacted Files	Coverage Δ
homeassistant/components/mqtt/vacuum.py	`92.46% <0%> (-1.92%)`	⬇️
homeassistant/components/mqtt/climate.py	`99.49% <0%> (-0.51%)`	⬇️
homeassistant/components/websocket_api/http.py	`89.34% <0%> (-0.18%)`	⬇️
homeassistant/util/yaml.py	`94.33% <0%> (-0.06%)`	⬇️
homeassistant/components/mqtt/discovery.py	`97.89% <0%> (-0.05%)`	⬇️
homeassistant/loader.py	`94.31% <0%> (-0.03%)`	⬇️
homeassistant/components/rflink/light.py	`100% <0%> (ø)`	⬆️
homeassistant/components/deconz/switch.py	`100% <0%> (ø)`	⬆️
homeassistant/components/litejet/switch.py	`100% <0%> (ø)`	⬆️
homeassistant/components/mqtt/binary_sensor.py	`100% <0%> (ø)`	⬆️
... and 130 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c314220...3d4fc29. Read the comment docs.

awarecan · 2019-04-14T22:39:11Z

You don't need specific other upstream lib, unless they are not pip-installed along with niko-home-control==0.2.0

awarecan

There is also a new pull request #23105 coming, what are the different between these two? Can you guys cooperate?

NoUseFreak · 2019-04-15T12:52:08Z

@awarecan I still have an issue where when I turn on a light, it takes a while for the state to be updated to the correct value. Should I be setting the state in the turn_on and turn_off methods?

It looks kind of weird if you want to turn something on and off again you have to click it multiple times before it turns off.

awarecan · 2019-04-15T14:04:10Z

You can set state in turn_on/off method, if you enabled assumed_state. We have tons of light/switch do this, we call it 'optimistic updates'

NoUseFreak · 2019-04-15T16:13:59Z

@awarecan Any way I can force a fetch after a state has changed in HA, I tried the assumed_state but it also did not update the state until a new fetch was done (30 sec later)

awarecan · 2019-04-15T16:33:58Z

assumed_state won't help you update the state, you need update state by yourself. See following example
https://github.com/home-assistant/home-assistant/blob/ec171b99286b52f9178fc15bade83d1cb7abe5dd/homeassistant/components/mysensors/switch.py#L137-L140

NoUseFreak · 2019-04-15T17:43:17Z

I'll focus on refactoring the underlying logic to provide a way to listen to the controller events on a later time. I first want my lights to turn on again. :-)
Any good examples you know off that have something like a callback function to update state from a long-running poller function?

codecov · 2019-04-16T07:27:00Z

Codecov Report

Merging #23093 into dev will decrease coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              dev   #23093     +/-   ##
=========================================
- Coverage   94.15%   93.96%   -0.2%     
=========================================
  Files         452      452             
  Lines       36809    36788     -21     
=========================================
- Hits        34658    34567     -91     
- Misses       2151     2221     +70

Impacted Files	Coverage Δ
homeassistant/components/demo/sensor.py	`0% <0%> (-100%)`	⬇️
homeassistant/components/demo/binary_sensor.py	`0% <0%> (-100%)`	⬇️
homeassistant/components/demo/device_tracker.py	`21.42% <0%> (-78.58%)`	⬇️
homeassistant/components/hassio/ingress.py	`67.76% <0%> (-10.29%)`	⬇️
homeassistant/components/axis/device.py	`93.02% <0%> (-6.98%)`	⬇️
homeassistant/components/deconz/config_flow.py	`97.77% <0%> (-2.23%)`	⬇️
homeassistant/components/axis/camera.py	`90.24% <0%> (-0.67%)`	⬇️
...assistant/components/manual/alarm_control_panel.py	`98.11% <0%> (-0.63%)`	⬇️
homeassistant/components/google_assistant/trait.py	`96.14% <0%> (-0.47%)`	⬇️
homeassistant/bootstrap.py	`58.78% <0%> (-0.24%)`	⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7102ea...2c29b75. Read the comment docs.

codecov · 2019-04-16T07:27:00Z

Codecov Report

Merging #23093 into dev will decrease coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              dev   #23093     +/-   ##
=========================================
- Coverage   94.15%   93.96%   -0.2%     
=========================================
  Files         452      452             
  Lines       36809    36788     -21     
=========================================
- Hits        34658    34567     -91     
- Misses       2151     2221     +70

Impacted Files	Coverage Δ
homeassistant/components/demo/sensor.py	`0% <0%> (-100%)`	⬇️
homeassistant/components/demo/binary_sensor.py	`0% <0%> (-100%)`	⬇️
homeassistant/components/demo/device_tracker.py	`21.42% <0%> (-78.58%)`	⬇️
homeassistant/components/hassio/ingress.py	`67.76% <0%> (-10.29%)`	⬇️
homeassistant/components/axis/device.py	`93.02% <0%> (-6.98%)`	⬇️
homeassistant/components/deconz/config_flow.py	`97.77% <0%> (-2.23%)`	⬇️
homeassistant/components/axis/camera.py	`90.24% <0%> (-0.67%)`	⬇️
...assistant/components/manual/alarm_control_panel.py	`98.11% <0%> (-0.63%)`	⬇️
homeassistant/components/google_assistant/trait.py	`96.14% <0%> (-0.47%)`	⬇️
homeassistant/bootstrap.py	`58.78% <0%> (-0.24%)`	⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7102ea...d0821e3. Read the comment docs.

Update base library and only pull for state once.

NoUseFreak · 2019-04-16T18:36:11Z

@awarecan Anything else I should do before it can be merged?

MartinHjelmare

Please open a new PR where we can address the comments.

MartinHjelmare · 2019-04-16T23:42:14Z


 PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
    vol.Required(CONF_HOST): cv.string,
+    vol.Optional(CONF_SCAN_INTERVAL, default=SCAN_INTERVAL): cv.time_period,


This is already part of the base platform schema.

MartinHjelmare · 2019-04-16T23:43:41Z

-        self._state = None
+        self._state = light.is_on
        self._brightness = None
+        _LOGGER.debug("Init new light: %s", light.name)


We try to avoid side effects in init method. Move the logging out of init.

MartinHjelmare · 2019-04-16T23:46:01Z

+        return self._unique_id
+
+    @property
+    def device_info(self):


device_info property is only read if the platform is loaded via config entry.

https://developers.home-assistant.io/docs/en/device_registry_index.html#defining-devices

MartinHjelmare · 2019-04-16T23:47:02Z

        return self._state

-    def turn_on(self, **kwargs):
+    async def async_turn_on(self, **kwargs):


Why change to async api? The device library doesn't support asyncio.

MartinHjelmare · 2019-04-16T23:49:18Z

+
+    def get_state(self, aid):
+        """Find and filter state based on action id."""
+        return next(filter(lambda a: a['id'] == aid, self.data))['value1'] != 0


Don't use filter + lambda. Use a generator expression.

NoUseFreak · 2019-04-17T21:39:00Z

@MartinHjelmare I fixed your feedback in #23176 together with upgrading the library version to make sure it installs it's dependencies.

homeassistant added integration: niko_home_control cla-signed labels Apr 14, 2019

ghost added the in progress label Apr 14, 2019

NoUseFreak force-pushed the fix-niko-home-control branch 5 times, most recently from 5b14f05 to 36165aa Compare April 14, 2019 11:00

NoUseFreak force-pushed the fix-niko-home-control branch from 36165aa to 3d4fc29 Compare April 14, 2019 11:48

NoUseFreak force-pushed the fix-niko-home-control branch 3 times, most recently from 01887cc to 71e450e Compare April 14, 2019 21:25

awarecan requested changes Apr 14, 2019

View reviewed changes

Comment thread homeassistant/components/niko_home_control/light.py Outdated

NoUseFreak force-pushed the fix-niko-home-control branch 2 times, most recently from 2573332 to a5891d5 Compare April 15, 2019 06:41

This comment has been minimized.

Sign in to view

pergolafabio mentioned this pull request Apr 15, 2019

Niko Home Control : TypeError: string indices must be integers #19283

Closed

NoUseFreak force-pushed the fix-niko-home-control branch from a5891d5 to f3e6973 Compare April 15, 2019 07:00

awarecan reviewed Apr 15, 2019

View reviewed changes

Comment thread homeassistant/components/niko_home_control/light.py Outdated

awarecan reviewed Apr 15, 2019

View reviewed changes

Comment thread homeassistant/components/niko_home_control/light.py Outdated

NoUseFreak force-pushed the fix-niko-home-control branch from f3e6973 to e751cc6 Compare April 15, 2019 12:34

NoUseFreak force-pushed the fix-niko-home-control branch from e751cc6 to ef2c223 Compare April 15, 2019 12:52

awarecan approved these changes Apr 15, 2019

View reviewed changes

awarecan reviewed Apr 15, 2019

View reviewed changes

Comment thread homeassistant/components/niko_home_control/light.py Outdated

NoUseFreak force-pushed the fix-niko-home-control branch from ef2c223 to 1e26e54 Compare April 15, 2019 14:52

NoUseFreak force-pushed the fix-niko-home-control branch from 1e26e54 to d8e294c Compare April 15, 2019 16:12

NoUseFreak force-pushed the fix-niko-home-control branch 3 times, most recently from b487ea6 to 8cfeb29 Compare April 15, 2019 17:39

NoUseFreak force-pushed the fix-niko-home-control branch 2 times, most recently from 302e30c to 2c29b75 Compare April 16, 2019 07:26

Fix niko_home_control integration

d0821e3

Update base library and only pull for state once.

NoUseFreak force-pushed the fix-niko-home-control branch from 2c29b75 to d0821e3 Compare April 16, 2019 08:23

awarecan merged commit 45e5f5d into home-assistant:dev Apr 16, 2019

awarecan removed the in progress label Apr 16, 2019

MartinHjelmare reviewed Apr 16, 2019

View reviewed changes

Uh oh!

Conversation

NoUseFreak commented Apr 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking Change:

Description:

Checklist:

Uh oh!

NoUseFreak commented Apr 14, 2019

Uh oh!

codecov Bot commented Apr 14, 2019

Codecov Report

Uh oh!

awarecan commented Apr 14, 2019

Uh oh!

awarecan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

NoUseFreak commented Apr 15, 2019

Uh oh!

Uh oh!

awarecan commented Apr 15, 2019

Uh oh!

NoUseFreak commented Apr 15, 2019

Uh oh!

awarecan commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NoUseFreak commented Apr 15, 2019

Uh oh!

codecov Bot commented Apr 16, 2019

Codecov Report

Uh oh!

codecov Bot commented Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

NoUseFreak commented Apr 16, 2019

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Apr 16, 2019

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Apr 16, 2019

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Apr 16, 2019

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Apr 16, 2019

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Apr 16, 2019

Choose a reason for hiding this comment

Uh oh!

NoUseFreak commented Apr 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

NoUseFreak commented Apr 14, 2019 •

edited

Loading

awarecan left a comment •

edited

Loading

awarecan commented Apr 15, 2019 •

edited

Loading

codecov Bot commented Apr 16, 2019 •

edited

Loading