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

Implement asynchronous notify() method #397

Merged
merged 20 commits into from
Jul 1, 2021
Merged

Implement asynchronous notify() method #397

merged 20 commits into from
Jul 1, 2021

Conversation

YoRyan
Copy link
Contributor

@YoRyan YoRyan commented Jun 19, 2021

Description:

Related issue (if applicable): #396

Adds an asynchronous notify() method to the Apprise object. Python 3 callers can use it like so:

async def main():
    apobj = Apprise()
    await apobj.async_notify(...)

...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:

  • Removal of each notification's ThreadPoolExecutor
  • Use of generators over lists where possible

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

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • No lint errors (use flake8)
  • 100% test coverage

@codecov-commenter
Copy link

codecov-commenter commented Jun 19, 2021

Codecov Report

Merging #397 (4373877) into master (a551079) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 4373877 differs from pull request most recent head 90d8d2a. Consider uploading reports for the commit 90d8d2a to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##            master      #397   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           94        94           
  Lines        11950     11961   +11     
  Branches      2005      2003    -2     
=========================================
+ Hits         11950     11961   +11     
Impacted Files Coverage Δ
apprise/Apprise.py 100.00% <100.00%> (ø)
apprise/py3compat/asyncio.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a551079...90d8d2a. Read the comment docs.

@caronc
Copy link
Owner

caronc commented Jun 19, 2021

@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 False returns are now changed to a raise TypeError ? You also have one test that was already re-written that had a return value of 0 (nothing to notify) and now suddenly it's notifying.

Also the naming of do_notify() seems a bit off; (but I understand this is the underlining call). It seems you really understand your async and Python (some of what you're doing i'm still trying to follow). Perhaps you can elaborate on some of your design ideas and thought process here?

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. 🙂 🙏

apprise/Apprise.py Show resolved Hide resolved
apprise/Apprise.py Show resolved Hide resolved
apprise/Apprise.py Show resolved Hide resolved
apprise/py3compat/asyncio.py Show resolved Hide resolved
apprise/Apprise.py Outdated Show resolved Hide resolved
@YoRyan
Copy link
Contributor Author

YoRyan commented Jun 19, 2021

Cheers! I've endeavored to answer all of your comments.

You also have one test that was already re-written that had a return value of 0 (nothing to notify) and now suddenly it's notifying.

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 None?

Also the naming of do_notify() seems a bit off; (but I understand this is the underlining call).

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 TypeError above. If there is anything else I can explain, please do ask!

YoRyan added 3 commits June 22, 2021 12:40
…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.
@caronc
Copy link
Owner

caronc commented Jun 23, 2021

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.

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 notify() to do_notify() which was... crazy... and as crazy as it was, that's not what happened. 🙂 So this is just me reading through your awesome work too quickly and misinterpreting what was done here. Disregard my comments about the function name.

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 @

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants