-
-
Notifications
You must be signed in to change notification settings - Fork 561
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
Add push server implementation to enable event handling #1446
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1446 +/- ##
==========================================
- Coverage 84.18% 83.35% -0.83%
==========================================
Files 135 139 +4
Lines 13448 13670 +222
Branches 1499 3248 +1749
==========================================
+ Hits 11321 11395 +74
- Misses 1909 2057 +148
Partials 218 218
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
@rytilahti This is ready for review/merge. I think it is now much cleaner more general and simpler. |
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.
Thanks for splitting it up, makes it much easier to review! I added some comments inline, alas, I haven't had time to setup my gateway for testing :(
A couple of things would be nice to have:
- Avoid bare dictionaries, and use some container classes (like attrs) that are then serialized to JSON
- Some tests would be nice, if you extract the payload generation into their own functions, you could at least test that given
EventInfo
s creates expected payloads to be send, subscribing and unsubscribing from events can also be tested without involving any testing on socket-level.
P.S. I updated the PR title to make it clearer for anyone reading the changelogs, hope that makes sense.
@rytilahti I think I have processed all feedback, could you have a look again/merge? |
@rytilahti I feel like this is really ready to be merged now. I feel like the tests can be added later, I would really like to focus on implementing it for the gateway next. |
Done, I revised and simplified the docs (e.g., by removing the details about how to perform traffic captures, which should actually be described somewhere in the docs..), and I removed also the full description of the Please give the docs a quick read, and adjust accordingly if I removed too much or if some piece of information is missing.
Okay, let's skip the tests for now. However, please move move the server into its own package, and extract separate classes into their own files. I think a directory structure like this would be good, and will allow adding tests later on without polluting the main package directory:
|
I read over the docs and made some small tweaks, I think they are now good to go. |
@rytilahti I restructured the files as you sugested, could you revieuw/merge? |
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.
LGTM, thanks @starkillerOG! 🎉
@rytilahti thank you for all the time you spent on reviewing and all the greath suggestions! |
Sure thing, please consider adding some tests afterwards at some point after you are done with the gateway implementation! I'll review the gateway impl PR as soon as possible, so that we can get it into the next release :-) |
This is a split out version of PR #1288.
It only includes the implementation of the push server to make it less confusing.
Test script to listen for alarm triggering of a aqara gateway:
Test script to register on a button press of aqara zigbee button connected to a aqara gateway: