-
-
Notifications
You must be signed in to change notification settings - Fork 442
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
Implement asynchronous notify() method #397
Conversation
Factor out the notification parsing logic from the notification sending logic, and cleanly separate the async from the non-async code. This is the first step toward implementing an asynchronous notify() method.
Send synchronous notifications immediately, inside synchronous code, instead of inside a coroutine.
Codecov Report
@@ Coverage Diff @@
## master #397 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 94 94
Lines 11950 11961 +11
Branches 2005 2003 -2
=========================================
+ Hits 11950 11961 +11
Continue to review full report at Codecov.
|
@YoRyan thank you for your contribution, I'll need some time to absorb it all and see how it still plays with Home Assistant. I'm a bit nervous how the Also the naming of Anyway, i'm certainly not discrediting anything you've done by any means at all.... seriously; it's just awesome! 👍 I don't get very many pull requests, so it's refreshing to see yours. 🙂 🙏 |
Cheers! I've endeavored to answer all of your comments.
Could you point me to the exact test and the exact notify call? Do you meant one of the ones that sends with a nonexistent tag and expects
Yeah, it's a tough function to name. The idea is that it allows both the synchronous and asynchronous tests to share common code, but have different ways of sending notifications. I've also clarified the use of |
…being sent These cases weren't being handled correctly by the existing try-except code. As a side benefit, the more_itertools dependency has been eliminated.
Given the current code, it is actually not possible to construct an Apprise object such that one server can successfully escape the body and title, but a subsequent server cannot.
In regard to the do_notify(); i am seeing very late in the game that naming was only for the unit tests 😊 . I had at first thought you renamed You've done outstanding work; i really appreciate it. 🚀 Just give me another day or 2 to look it over (sorry for being so slow); it's just such a major change. I'm concerned about Home Assistant, but i'll do a few more checks (i haven't gotten the warning yet; but i didn't get it last time either, it was @ |
Description:
Related issue (if applicable): #396
Adds an asynchronous notify() method to the Apprise object. Python 3 callers can use it like so:
...and, the current configuration permitting, notifications will be dispatched asynchronously using the same event loop as that of the caller.
To make this happen, I had to perform a major refactor of much of the existing async code. Some improvements include:
Don't hesitate to ask if you'd like something clarified! I've tested this PR successfully on Python 2.7 and 3.9.
Checklist
flake8
)