Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Netatmo, address comments from #20755 #21157

Merged
merged 2 commits into from Feb 19, 2019

Conversation

Projects
None yet
3 participants
@danielperna84
Copy link
Contributor

danielperna84 commented Feb 17, 2019

Description:

Address comments from @MartinHjelmare in PR #20755 .
I got rid of the global NETATMO_PERSONS. I can't remove the NETATMO_AUTH because the climate component also uses that, and I don't have such a device. Thus I can't verify if it will keep working.

What I did not change yet is the comment regarding the usage of hass.components.webhook.async_generate_id and hass.components.webhook.async_generate_url within the sync setup_platform. I didn't find any other component where I could steal that from, and I don't know enough about async to know what the correct solution should be. IMHO there should actually a sync-safe variant for other sync-components to use.

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.
NETATMO_WEBHOOK_URL = hass.components.webhook.async_generate_url(
webhook_id)
hass.data[
DATA_WEBHOOK_URL] = hass.components.webhook.async_generate_url(

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 17, 2019

Member

There's actually nothing in these webhook callback decorated helpers that makes them necessary to be run in the event loop.

So technically we can just call them here directly, even though it's not according to our standards.

Otherwise I think we would have to refactor netatmo more and use a coroutine function for component setup.

This comment has been minimized.

Copy link
@danielperna84

danielperna84 Feb 18, 2019

Author Contributor

Uhm, refactoring to async might be out of scope for this PR. So if you say technically it's not a problem, then the changes I have made would address all the other issues you have commented on. 🤔

This comment has been minimized.

Copy link
@danielperna84

danielperna84 Feb 18, 2019

Author Contributor

Oh, fixed the remaining lint error.

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Good! Can be merged when build passes.

@danielperna84 danielperna84 merged commit 9d3eaad into home-assistant:dev Feb 19, 2019

3 checks passed

Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Feb 19, 2019

@danielperna84 danielperna84 deleted the danielperna84:netatmo branch Feb 19, 2019

@balloob balloob referenced this pull request Mar 6, 2019

Merged

0.89.0 #21712

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.