Skip to content

deCONZ use forward entry setup#13990

Merged
balloob merged 15 commits intohome-assistant:devfrom
Kane610:deconz-use-forward-entry-setup
Apr 23, 2018
Merged

deCONZ use forward entry setup#13990
balloob merged 15 commits intohome-assistant:devfrom
Kane610:deconz-use-forward-entry-setup

Conversation

@Kane610
Copy link
Copy Markdown
Member

@Kane610 Kane610 commented Apr 18, 2018

Description:

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

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

@Kane610
Copy link
Copy Markdown
Member Author

Kane610 commented Apr 18, 2018

@balloob just a quick check I'm not doing something different from how you want it. Also, are tests expected for the platforms and deconz setup?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Writing auth credentials to the log

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There are a couple of places in the lib that also happens, but only when enabling debug. Another issue is that all communication is http only.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any reason this is a standalone method and not just part of async_setup_entry ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Only legacy reasons, which are now irrelevant. Thanks for pointing that out!

@balloob
Copy link
Copy Markdown
Member

balloob commented Apr 18, 2018

Yes this seems right.

And yes, everything that has to do with config entries or entity registry requires full test coverage.

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Apr 19, 2018

For another PR we should check in entity platform helper if the platform has setup_platform attribute, and only then call that. Then we can remove these functions from platforms that don't need them and are set up via config entry forwarding.

Comment thread tests/components/deconz/test_init.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

redefinition of unused 'test_setup_entry_already_registered_bridge' from line 71

@balloob
Copy link
Copy Markdown
Member

balloob commented Apr 21, 2018

Make sure to remove the WIP from the title when it's ready for review.

@Kane610
Copy link
Copy Markdown
Member Author

Kane610 commented Apr 21, 2018

@balloob it's the tests that are left. Trying to figure stuff out. Would help if you could hint me in the right direction.

I'm trying to figure out how to patch/mock the pydeconz class and methods since they only need to return true or false with async setup entry

Comment thread tests/components/scene/test_deconz.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

too many blank lines (2)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (86 > 79 characters)

Comment thread tests/components/light/test_deconz.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

Comment thread tests/components/sensor/test_deconz.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (83 > 79 characters)

@Kane610 Kane610 changed the title WIP: deCONZ use forward entry setup deCONZ use forward entry setup Apr 22, 2018
@Kane610
Copy link
Copy Markdown
Member Author

Kane610 commented Apr 22, 2018

@balloob I feel like this is ready for a review now

@Kane610 Kane610 force-pushed the deconz-use-forward-entry-setup branch from 85a8278 to e1069ae Compare April 22, 2018 21:23
@balloob balloob merged commit 8a10fcd into home-assistant:dev Apr 23, 2018
@balloob
Copy link
Copy Markdown
Member

balloob commented Apr 23, 2018

Awesome!

@Kane610
Copy link
Copy Markdown
Member Author

Kane610 commented Apr 23, 2018

Great @balloob !

@balloob balloob mentioned this pull request May 11, 2018
@Kane610 Kane610 deleted the deconz-use-forward-entry-setup branch May 18, 2018 21:18
@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants