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

Move request sync logic into GoogleConfig #28227

Merged
merged 12 commits into from Nov 26, 2019

Conversation

@elupus
Copy link
Contributor

elupus commented Oct 26, 2019

Description:

This moves the request_sync logic into same place as cloud version has the request sync logic.

I have been forced to extend base class to send along the useragent identifier. Since that is supported in the local case.

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.
  • I have followed the development checklist

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

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

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

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@probot-home-assistant

This comment has been minimized.

Copy link

probot-home-assistant bot commented Oct 26, 2019

Hey there @home-assistant/cloud, mind taking a look at this pull request as its been labeled with a integration (google_assistant) you are listed as a codeowner for? Thanks!

@probot-home-assistant

This comment has been minimized.

Copy link

probot-home-assistant bot commented Oct 26, 2019

Hey there @home-assistant/cloud, mind taking a look at this pull request as its been labeled with a integration (cloud) you are listed as a codeowner for? Thanks!

@elupus

This comment was marked as outdated.

Copy link
Contributor Author

elupus commented Oct 26, 2019

I've dropped return value on request_sync calls. Nothing ever used it. Also cloud could raise a timeout error, so exception returns are already in place for error situations. Seems cloud also should use raise for status.

I've reverted this change

@MartinHjelmare MartinHjelmare moved this from Needs review to Incoming in Dev Oct 26, 2019
Dev automation moved this from Incoming to Reviewer approved Oct 28, 2019
@balloob

This comment has been minimized.

Copy link
Member

balloob commented Oct 28, 2019

Looks good.

@elupus elupus marked this pull request as ready for review Oct 28, 2019
@elupus elupus force-pushed the elupus:google_assistant_storage branch from c812ddd to 1275941 Nov 15, 2019
@elupus elupus force-pushed the elupus:google_assistant_storage branch from 1275941 to 16e0405 Nov 15, 2019
elupus added 6 commits Nov 15, 2019
async def _schedule_callback(self, _now):
"""Handle a scheduled sync callback."""
self._google_sync_unsub = None
await self.async_sync_entities()
return await self._async_request_sync_devices(agent_user_id)

@callback
def async_schedule_google_sync(self):
def async_schedule_google_sync(self, agent_user_id: str):
"""Schedule a sync."""
if self._google_sync_unsub:
self._google_sync_unsub()

self._google_sync_unsub = async_call_later(
self.hass, SYNC_DELAY, self._schedule_callback
async def _schedule_callback(_now):
"""Handle a scheduled sync callback."""
self._google_sync_unsub.pop(agent_user_id)
await self.async_sync_entities(agent_user_id)

self._google_sync_unsub.pop(agent_user_id, lambda: None)()

self._google_sync_unsub[agent_user_id] = async_call_later(
self.hass, SYNC_DELAY, _schedule_callback
Comment on lines 132 to 142

This comment has been minimized.

Copy link
@elupus

elupus Nov 15, 2019

Author Contributor

Had to change this to the parameteric agent_user_id case too if we are to use fixed agent_user_id. Will be used by upcoming stored version.

@elupus

This comment has been minimized.

Copy link
Contributor Author

elupus commented Nov 15, 2019

@balloob had to do small refactor of the way scheduled sync is handled to allow for the case of required agent_user_id. It now keeps track of scheduled sync calls per agent as well. I'll merge in a few days if you have no objections.

self._google_sync_unsub.pop(agent_user_id)
await self.async_sync_entities(agent_user_id)

self._google_sync_unsub.pop(agent_user_id, lambda: None)()

This comment has been minimized.

Copy link
@balloob

balloob Nov 26, 2019

Member

You're saving 1 line of code but create an empty lambda to invoke 🤷‍♂

Copy link
Member

balloob left a comment

Ok to merge when last comment addressed

Co-Authored-By: Paulus Schoutsen <paulus@home-assistant.io>
@elupus elupus merged commit 06231e7 into home-assistant:dev Nov 26, 2019
10 of 11 checks passed
10 of 11 checks passed
codecov/patch 76.47% of diff hit (target 94.43%)
Details
CI #20191126.97 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/project 94.5% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Nov 26, 2019
@lock lock bot locked and limited conversation to collaborators Nov 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
3 participants
You can’t perform that action at this time.