Add Dynalite switch platform by ziv1234 · Pull Request #32389 · home-assistant/core

ziv1234 · 2020-03-01T23:37:00Z

Breaking change

Not a breaking change - backwards compatible

Proposed change

this is the next phase in the Dynalite integration. Now that the component is refactored, adding the switch is simple

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example entry for `configuration.yaml`:

# Example configuration.yaml
dynalite:
  bridges:
    - host: DEVICE_IP_ADDRESS
      port: 12345
      autodiscover: true
      polltimer: 1
      area:
        '1':
          name: Office
        '2':
          name: Living Room
          nodefault: true
          channel:
            '2': 
              name: Entrance Spot
              fade: 10.0
            '3': 
              name: Dining Table
          preset:
            '5':
              name: Blinking Lights
            '6':
              name: All Off
              fade: 3.0
      preset:
        '1':
          name: 'On'
        '4':
          name: 'Off'

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Dynalite - addition of switch platform home-assistant.io#12227

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

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

Documentation added/updated for www.home-assistant.io

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

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

codecov · 2020-03-02T00:18:31Z

Codecov Report

Merging #32389 into dev will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##              dev   #32389      +/-   ##
==========================================
+ Coverage   94.75%   94.75%   +<.01%     
==========================================
  Files         776      777       +1     
  Lines       56174    56195      +21     
==========================================
+ Hits        53227    53248      +21     
  Misses       2947     2947

Impacted Files	Coverage Δ
homeassistant/components/dynalite/const.py	`100% <100%> (ø)`	⬆️
homeassistant/components/dynalite/bridge.py	`100% <100%> (ø)`	⬆️
homeassistant/components/dynalite/config_flow.py	`100% <100%> (ø)`	⬆️
homeassistant/components/dynalite/switch.py	`100% <100%> (ø)`

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 c361358...38b3c6a. Read the comment docs.

balloob · 2020-03-04T22:43:54Z

+    """Record the async_add_entities function to add them later when received from Dynalite."""
+
+    @callback
+    def switch_from_device(device, bridge):


Why even this and not pass DynaliteSwitch directly ?

what do you mean? pass it in the function?
async_setup_entry_base(
hass, config_entry, async_add_entities, "switch", DynaliteSwitch
)

done. also in light.py

balloob · 2020-03-04T22:50:58Z

        vol.Optional(CONF_DEFAULT): PLATFORM_DEFAULTS_SCHEMA,
-        vol.Optional(CONF_ACTIVE, default=False): vol.Coerce(bool),
+        vol.Optional(CONF_ACTIVE, default=False): vol.Any(
+            CONF_ACTIVE_ON, CONF_ACTIVE_OFF, CONF_ACTIVE_INIT, bool


bool should probably be cv.boolean

Also, isn't it confusing that you allow string values and boolean values here ?

I wanted to keep it backwards compatible since there is a third option. I can move it to only the CONF_* as true and false get routed to on and off

ok, makes sense.

MartinHjelmare · 2020-03-05T07:51:41Z

        return await self.dynalite_devices.async_setup()

-    async def reload_config(self, config):
+    def reload_config(self, config):


We're still awaiting this function in the entry update listener.

I know. the reload_config is a sync function, and the library is built in a way that reload_config can be called at any time, as long as it is called from the same thread of the event loop.
However, I didn't like this too much either, as it is confusing, but couldn't find another solution, so definitely open to any suggestions. My problem is that if the entry is already defined, but the config has changed, I need to call the update. However, the bridge is not yet fully set up at this point as this happens in parallel. If there is a way to wait until the config entry is set up before loading the new config from configuration.yaml, then I would only call it at that time. However, all the solutions I could think off were even uglier, so would appreciate any insight on another component that is doing it right and I could use as reference.
Note that it does work and I couldnt find any bugs in the flow, but it is not the best design pattern and will happily change it if I can find a cleaner solution

I don't think this can work. It's not possible to await non awaitable functions.

Why not? reload_config does not await on anything. It only updates internal data structures, which are designed that they could change during any await in the async_setup (or others), so it does not depend on async_setup being completed.
That said, I like your suggestion below, which will make it possible to write a clean code that does not require explanations (and therefore also safer). Will give it a shot tonight

We're still awaiting this function, that's the problem.

you are correct. my mistake. fixed in the PR I am preparing

just found out why it wasn't caught in the tests. it still behaved "ok" since it evaluated "bridge.reload_config(entry.data)" as it is a sync function. then it tried to await its return value, None, and threw an assertion. However, since this is the last meaningful statement (except a log), the test still showed that everything happened.
BTW, I believe the code would also behave well in that case except for the log about the exception, but obviously i fixed it to remove the await.
I was wondering if there is a way to make pytest fail in case there are uncaught exception, not awaited coroutines, etc. to avoid these instances and verify a cleaner code

Non awaited coroutines should show up in the warning log when running pytest I think.

But here it's the other way, right? We're awaiting when we shouldn't. Not sure about that one.

that is correct. it is not a good pattern anyway and i fixed it. It throws an exception. The problem is that the test succeeds. I would like a way to fail it on assertions / non awaited, etc. as i shouldn't have them in the code
BTW, it can be a bit tricky since i get many tests with an uncaught exception for persistent_notification/create:
ERROR homeassistant.core:core.py:143 Error doing job: Task exception was never retrieved
Traceback (most recent call last):
File "/home/ziv/switch/home-assistant/homeassistant/core.py", line 1209, in async_call
raise ServiceNotFound(domain, service) from None
homeassistant.exceptions.ServiceNotFound: Unable to find service persistent_notification/dismiss

I am not doing anything directly with persistent_notification, but it does seem to me that tests should complete with no warnings at all

MartinHjelmare · 2020-03-05T07:52:44Z

    bridge = DynaliteBridge(hass, entry.data)
+    # need to do it before the listener
+    hass.data[DOMAIN][entry.entry_id] = bridge
    entry.add_update_listener(async_entry_changed)


Why add the listener before the bridge is set up?

For the same reason listed above. If the entry was already set up with some config, but in the next restart of HA, the configuration.yaml file has changed, the listener will need to be called as the entry is being changed and it is happening in parallel with the setup of the original entry, which has not completed yet. If I can find a better solution for the first one, will hopefully be also able to solve this

Perhaps store the bridge.async_setup task, id it with the config entry id and await the task before updating the entry.

that could work. Should i store it in hass.data[entry_id]?

i removed the await and it should be ok now. When I try this, it complicates things since if I await, it is still not guaranteed that the hass.data[] has been updated, depending on which gets executed first

Let's discuss more in the PR for this.

ziv1234 added 2 commits March 1, 2020 22:40

added presets for switch devices

2f9c2bf

added channel type to __init and const

fd97272

homeassistant added the cla-signed label Mar 1, 2020

probot-home-assistant Bot added has-tests integration: dynalite new-platform by-code-owner labels Mar 1, 2020

ziv1234 mentioned this pull request Mar 1, 2020

Dynalite - addition of switch platform home-assistant/home-assistant.io#12227

Merged

7 tasks

ran pylint on library so needed a few changes in names

d00e943

MartinHjelmare changed the title ~~Dynalite - addition of switch platform~~ Add Dynalite switch platform Mar 3, 2020

balloob reviewed Mar 4, 2020

View reviewed changes

balloob approved these changes Mar 4, 2020

View reviewed changes

balloob reviewed Mar 4, 2020

View reviewed changes

ziv1234 added 2 commits March 4, 2020 23:50

removed callback

64d7a6d

bool -> cv.boolean

38b3c6a

balloob merged commit 521cc72 into home-assistant:dev Mar 5, 2020

MartinHjelmare reviewed Mar 5, 2020

View reviewed changes

lock Bot locked and limited conversation to collaborators Mar 11, 2020

Uh oh!

Conversation

ziv1234 commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

Uh oh!

codecov Bot commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

MartinHjelmare Mar 5, 2020 • 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!

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!

MartinHjelmare Mar 5, 2020 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ziv1234 commented Mar 1, 2020 •

edited

Loading

Example entry for `configuration.yaml`:

codecov Bot commented Mar 2, 2020 •

edited

Loading

MartinHjelmare Mar 5, 2020 •

edited

Loading

MartinHjelmare Mar 5, 2020 •

edited

Loading