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

Refactor Alexa Smart Home API #12016

Merged
merged 1 commit into from Jan 29, 2018
Merged

Refactor Alexa Smart Home API #12016

merged 1 commit into from Jan 29, 2018

Conversation

bitglue
Copy link
Member

@bitglue bitglue commented Jan 28, 2018

Description:

No functionality changes here. Just a refactor to lay the groundwork for #11973 .

Having an object per interface will make it easier to support
properties.

Ideally, properties are reported in context in all responses. However
current implementation reports them only in response to a ReportState
request. This seems to work sufficiently. As long as the device is
opened in the Alexa app, Amazon will poll the device state every few
seconds with a ReportState request.

Checklist:

  • The code change is tested and works locally.
  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

Having an object per interface will make it easier to support
properties.

Ideally, properties are reported in context in all responses. However
current implementation reports them only in response to a ReportState
request. This seems to work sufficiently. As long as the device is
opened in the Alexa app, Amazon will poll the device state every few
seconds with a ReportState request.
'supported': self.properties_supported(),
'proactivelyReported': self.properties_proactively_reported(),
},
# XXX this is incorrect, but the tests assert it
Copy link
Member

Choose a reason for hiding this comment

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

If it's incorrect, let's remove it. Shouldn't hurt our interaction with Alexa?

Copy link
Member

@balloob balloob Jan 28, 2018

Choose a reason for hiding this comment

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

I see that you have fixed this in #11973 but I don't want the dev branch to be in a broken state, so let's include the fix in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was broken before -- I just didn't notice. I can re-arrange.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, in that case it's fine.

Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Ok to merge once the comment has been addressed.

@balloob balloob merged commit 84711aa into home-assistant:dev Jan 29, 2018
@balloob balloob mentioned this pull request Feb 9, 2018
@balloob balloob mentioned this pull request Feb 21, 2018
2 tasks
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 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.

None yet

3 participants