-
-
Notifications
You must be signed in to change notification settings - Fork 37.5k
Update Ring to 0.6.0 #30748
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
Merged
Merged
Update Ring to 0.6.0 #30748
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
f3e0c78
Update Ring to 0.6.0
balloob c6ec1b6
Update sensor tests
balloob aa85275
update -> async_update
balloob e57db9a
Delete temp files
balloob ee3ddc8
Address comments
balloob 30394d3
Final tweaks
balloob 34a22dc
Remove stale print
balloob File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this better than using a dict cache and our
Throttlehelper to throttle updating the cache?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, because with the
Throttledecorator, if a call is in process, it will returnNoneinstead of waiting for the call to finish.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So at worst we would then get 10 second stale data?
What kind of data is this?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last 10 events for a device.
Throttle returns
None, not the data. The implementation would become thendata.update(); handle(data.data)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if this approach seems to work, we might want to turn that into a generic asyncio throttle function. We do a similar thing in Hue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still not clear to me why a central update and cache with signals telling the entities to update would be worse than this central update with a timed cache where entities pull data, in this situation.
The entities per device that are involved are fixed, except for the camera where only cameras that have subscription are added. So there won't be less or more entities per device pulling data except for the camera case. Maybe the camera entity is the reason?
My point is, that if we know what devices we have and nothing about the features of those devices changes, we should be able to update all devices regardless of platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my bad. I thought you asked why not use X, instead of how does it work with the existing dispatch update that Ring uses.
So I don't have a good reason. And it is indeed kinda complicated now. We now have a health tracker that has an opt-in signal. We could have used the same approach for history. hmm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to rewrite it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok like this. I mostly want to understand the reasons for the different designs so I know when to recommend what approach. And I like as few choices as possible for my recommendations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about my last comment tone. I meant to say want ME to rewrite it.
I thought about it more and I'll do one more pass cleaning up data fetching. I can do better here.