Enhance Dynalite Integration after review by ziv1234 · Pull Request #31760 · home-assistant/core

ziv1234 · 2020-02-12T19:42:28Z

Breaking change

Proposed change

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)
[ x] Code quality improvements to existing code or addition of tests

Example entry for `configuration.yaml`:

# Example configuration.yaml

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Fixed docs per changes in PR #31760 home-assistant.io#12157

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

ziv1234 · 2020-02-13T07:50:49Z

all tests pass now include coverage. Should be ready to merge as this is just a code quality change. No functionality is affected

MartinHjelmare

Please always use the core interfaces in the tests, and never the integration interfaces directly. Doing this will make the tests more robust, and we'll be able to change the implementation without changing the tests as long as the result in the core remains the same.

Co-Authored-By: Martin Hjelmare <marhje52@gmail.com>

changed test_init to use the core methods

ziv1234 · 2020-02-18T18:46:57Z

The problem is that it only updates if there are more items there.

What do you mean?

not updates.items() <= entry.data.items()

wouldn't it only work if there are more items in the new config?

Anyway, if the DATA_CONFIGS pattern is ok, I can easily implement it correctly. Will do so now, so please let me know whether or not to push

MartinHjelmare · 2020-02-18T19:06:01Z

I think that will just check if the existing data is the same as the updates and not update if it's the same.

I think we should use my suggested approach and not do anything extra.

ziv1234 · 2020-02-18T19:27:08Z

I think that will just check if the existing data is the same as the updates and not update if it's the same.

I think we should use my suggested approach and not do anything extra.

sure. done

MartinHjelmare

Looks good! Nice work!

ziv1234 · 2020-02-18T21:15:43Z

Looks good! Nice work!

Thanks! Appreciate all the work you did. Learned a lot about Home Assistant.

Now I can add the switch and cover platforms in another pull request after this one is merged

frenck · 2020-02-21T13:58:44Z

The host isn't a unique ID? It can change...
So you are using host as the unique ID and want to update it with the same thing that is the unique ID? That will never work 🤷‍♂

For example, when if you run multiple on different ports and the same host? Proxies?

IMHO, a unique ID should not be used here.

MartinHjelmare · 2020-02-21T14:14:33Z

This is just a unique id for the config entry not the entities. The config entry would need to be replaced anyway if the host would change. Otherwise the integration would stop working.

ziv1234 · 2020-02-21T14:49:18Z

The host is the most constant thing. This is the IP of the gateway. There is more info in the config entry so I update it with the same host and the rest of the data. You can see what I put on the issue I opened. Is there another place in has to store persistent data?

frenck · 2020-02-21T14:55:13Z

I've seen your issue @ziv1234.

I think this has mainly to do with existing entries, that don't have a unique ID registered yet, but are in the registry.

You can solve that by updating the entry on entry setup, like so:

https://github.com/home-assistant/home-assistant/blob/dd8597cb46c5e788995e1ea56b87dfbaf43ca51c/homeassistant/components/wled/__init__.py#L64-L68

ziv1234 · 2020-02-21T15:29:54Z

maybe i misunderstood something, but it seems like they are handling something else. In wled they are looking at an entry that doesn't have unique_id so they configure one for it. In my case, all entries will have unique_id, but they will be running with a different data (entry.data) which is what I expected to update.

In dynalite, I use entry.data["host"] as the unique_id, but the other parameters (port, areas / devices, etc.) could change, so would expect to keep the unique_id and update it.

It works, but the problem is that it requires and extra restart of HA.

Is there something I should do differently here?

MartinHjelmare · 2020-02-21T16:22:41Z

The config entry will be updated immediately before aborting the config flow when checking for unique config entry id.

If you need to take some action in the integration when the config entry data is updated, I think you have to register a listener.

https://developers.home-assistant.io/docs/config_entries_options_flow_handler#signal-updates

I think this listener is required if the config entry already has been loaded when the update happens.

MartinHjelmare · 2020-02-21T16:24:30Z

Marking this for 0.106 since the first PR for this integration was merged before beta cut and this PR is cleaning up many things about the first PR.

ziv1234 · 2020-02-21T18:04:32Z

ok. will start thinking about how to implement the signal update one. It makes sense.

Is there a way to ask home assistant to unload and reload an entry?

MartinHjelmare · 2020-02-21T20:33:42Z

Yes, hass.config_entries.async_reload.

https://github.com/home-assistant/home-assistant/blob/2fb66fd866b39f210f48edb14d3e8b21f62ff974/homeassistant/config_entries.py#L705

MartinHjelmare · 2020-02-21T20:34:28Z

Can we merge here now, and further changes can go in a future PR?

MartinHjelmare · 2020-02-21T20:35:31Z

Btw, we need to update the docs, since we removed some config items. Please link a docs PR in the PR description. Then we can merge.

ziv1234 · 2020-02-21T22:09:46Z

done. created PR home-assistant/home-assistant.io#12157

ziv1234 · 2020-02-21T22:12:36Z

we can merge now. i believe that the change to use reload_entry could be tricky, and the problem only happens when changing configuration.yaml, which shouldn't happen often after the initial setup

* fixes per Martin Hjelmare * pylint fix * final fixes per request * fixed unit tests for new config flow * Added unit-tests to increase coverage. at 97% now * Added unit tests for 100% coverage of component * removed configured_host function and updated config_flow unit tests * added a pylint directive since it tells me by mistake DOMAIN is not used * fixed path (removed __init__) * Update homeassistant/components/dynalite/light.py Co-Authored-By: Martin Hjelmare <marhje52@gmail.com> * Update homeassistant/components/dynalite/light.py Co-Authored-By: Martin Hjelmare <marhje52@gmail.com> * fixed the test as we moved from schedule_update_... to async_schedule * Update homeassistant/components/dynalite/bridge.py Co-Authored-By: Martin Hjelmare <marhje52@gmail.com> * removed context from config_flow changed test_init to use the core methods * moved test_light to also use the core interfaces * moved to config_entries.async_unload * additional fixes for the tests * pylint fix and removed unnecessary code * Update tests/components/dynalite/test_light.py Co-Authored-By: Martin Hjelmare <marhje52@gmail.com> * Update tests/components/dynalite/test_light.py Co-Authored-By: Martin Hjelmare <marhje52@gmail.com> * Update tests/components/dynalite/test_light.py Co-Authored-By: Martin Hjelmare <marhje52@gmail.com> * Update tests/components/dynalite/test_light.py Co-Authored-By: Martin Hjelmare <marhje52@gmail.com> * Update tests/components/dynalite/test_light.py Co-Authored-By: Martin Hjelmare <marhje52@gmail.com> * Update tests/components/dynalite/test_light.py Co-Authored-By: Martin Hjelmare <marhje52@gmail.com> * Update tests/components/dynalite/test_light.py Co-Authored-By: Martin Hjelmare <marhje52@gmail.com> * Update tests/components/dynalite/test_light.py Co-Authored-By: Martin Hjelmare <marhje52@gmail.com> * Update tests/components/dynalite/test_light.py Co-Authored-By: Martin Hjelmare <marhje52@gmail.com> * added break in loop * removed last mock_coro reference pylint fix * added coverage for try_connect * added check for a successful connection before bridge.async_setup succeeds also added a "nowait" config option (default False) that avoids this check * changed log level * fixed accidental chmod I did * fixed accidental change * not storing config in bridge * not patching asyncio * moved CONFIG_SCHEMA into component * moved all logs to start capitalized (and revised some of them) * moved test_config_flow to not patch the DynaliteBridge * also took DynaliteBridge patching out of test_init * removed NO_WAIT * fixes to SCHEMA * changed _ in multi-word CONF moved imports to component const.py * removed tries * removed redundant tests * fixed some small change i broke in the library. only version update * fixed rewuirements * Update tests/components/dynalite/test_config_flow.py Co-Authored-By: Martin Hjelmare <marhje52@gmail.com> * Update tests/components/dynalite/test_light.py Co-Authored-By: Martin Hjelmare <marhje52@gmail.com> * Update tests/components/dynalite/test_config_flow.py Co-Authored-By: Martin Hjelmare <marhje52@gmail.com> * removed HIDDEN_ENTITY removed hass in test fixture * black fixes * removed final piece of hidden_entity from light fix in the library updated config flow so if the entry is already set but with a different config, calls async_update_entry * removed DATA_CONFIGS - no longer necessary * pylint fixes * added coverage * use abort in config_flow * test update * removed logs * test that update actually updates the entry Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

fixes per Martin Hjelmare

990f7c7

homeassistant added the cla-signed label Feb 12, 2020

probot-home-assistant Bot added has-tests integration: dynalite by-code-owner labels Feb 12, 2020

ziv1234 added 2 commits February 12, 2020 20:03

pylint fix

c60b5b3

final fixes per request

9018a3b

ziv1234 mentioned this pull request Feb 12, 2020

Dynalite Integration #27841

Merged

9 tasks

MartinHjelmare changed the title ~~Dynalite Integration - fixes per Martin Hjelmare's asks~~ Enhance Dynalite Integration after review Feb 12, 2020

MartinHjelmare self-assigned this Feb 12, 2020

ziv1234 added 2 commits February 12, 2020 21:01

fixed unit tests for new config flow

e74acf9

Added unit-tests to increase coverage. at 97% now

084da2f

This comment has been minimized.

Sign in to view

Added unit tests for 100% coverage of component

4d7c1c0

MartinHjelmare requested changes Feb 13, 2020

View reviewed changes

MartinHjelmare reviewed Feb 13, 2020

View reviewed changes

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

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

Comment thread homeassistant/components/dynalite/bridge.py Outdated

Comment thread homeassistant/components/dynalite/config_flow.py Outdated

ziv1234 and others added 9 commits February 13, 2020 19:43

removed configured_host function and updated config_flow unit tests

7f3e015

added a pylint directive since it tells me by mistake DOMAIN is not used

bb58c48

fixed path (removed __init__)

7dac86b

Update homeassistant/components/dynalite/light.py

9032162

Co-Authored-By: Martin Hjelmare <marhje52@gmail.com>

Update homeassistant/components/dynalite/light.py

ddb16a0

Co-Authored-By: Martin Hjelmare <marhje52@gmail.com>

fixed the test as we moved from schedule_update_... to async_schedule

6a0606b

Update homeassistant/components/dynalite/bridge.py

dc683ac

Co-Authored-By: Martin Hjelmare <marhje52@gmail.com>

removed context from config_flow

21fb245

changed test_init to use the core methods

Merge branch 'dev' of https://github.com/ziv1234/home-assistant into dev

bd33b26

MartinHjelmare reviewed Feb 14, 2020

View reviewed changes

ziv1234 added 3 commits February 14, 2020 11:49

moved test_light to also use the core interfaces

ed99d82

moved to config_entries.async_unload

0081599

additional fixes for the tests

a768ed1

use abort in config_flow

5af5bc5

MartinHjelmare reviewed Feb 18, 2020

View reviewed changes

Comment thread tests/components/dynalite/test_config_flow.py

ziv1234 added 3 commits February 18, 2020 19:54

test update

9d35e5f

removed logs

4868e96

test that update actually updates the entry

ba6cd6b

MartinHjelmare approved these changes Feb 18, 2020

View reviewed changes

ziv1234 mentioned this pull request Feb 21, 2020

._abort_if_unique_id_configured(update) doesn't update #32059

Closed

MartinHjelmare added this to the 0.106.0 milestone Feb 21, 2020

MartinHjelmare merged commit 36db302 into home-assistant:dev Feb 21, 2020

ziv1234 mentioned this pull request Feb 21, 2020

Fixed docs per changes in PR #31760 home-assistant/home-assistant.io#12157

Merged

7 tasks

balloob added the cherry-picked label Feb 22, 2020

lock Bot locked and limited conversation to collaborators Feb 23, 2020

Uh oh!

Conversation

ziv1234 commented Feb 12, 2020 • edited by MartinHjelmare 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!

This comment has been minimized.

ziv1234 commented Feb 13, 2020

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ziv1234 commented Feb 18, 2020

Uh oh!

MartinHjelmare commented Feb 18, 2020

Uh oh!

ziv1234 commented Feb 18, 2020

Uh oh!

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

ziv1234 commented Feb 18, 2020

Uh oh!

frenck commented Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartinHjelmare commented Feb 21, 2020

Uh oh!

ziv1234 commented Feb 21, 2020

Uh oh!

frenck commented Feb 21, 2020

Uh oh!

ziv1234 commented Feb 21, 2020

Uh oh!

MartinHjelmare commented Feb 21, 2020

Uh oh!

MartinHjelmare commented Feb 21, 2020

Uh oh!

ziv1234 commented Feb 21, 2020

Uh oh!

MartinHjelmare commented Feb 21, 2020

Uh oh!

MartinHjelmare commented Feb 21, 2020

Uh oh!

MartinHjelmare commented Feb 21, 2020

Uh oh!

ziv1234 commented Feb 21, 2020 • edited by MartinHjelmare Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ziv1234 commented Feb 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ziv1234 commented Feb 12, 2020 •

edited by MartinHjelmare

Loading

Example entry for `configuration.yaml`:

frenck commented Feb 21, 2020 •

edited

Loading

ziv1234 commented Feb 21, 2020 •

edited by MartinHjelmare

Loading