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

Fix notify() failing when being called from an existing asynchronous event loop #624

Merged
merged 2 commits into from
Jul 13, 2022

Conversation

YoRyan
Copy link
Contributor

@YoRyan YoRyan commented Jul 7, 2022

Description:

Related issue (if applicable): #610

If an event loop is already in use, use it--instead of creating a new one, which fails with the high-level asyncio.run() function. Test included.

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 Jul 7, 2022

Codecov Report

Merging #624 (314b5e6) into master (113fa36) will decrease coverage by 0.72%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##            master     #624      +/-   ##
===========================================
- Coverage   100.00%   99.27%   -0.73%     
===========================================
  Files          110      110              
  Lines        14266    14274       +8     
  Branches      2700     2700              
===========================================
- Hits         14266    14170      -96     
- Misses           0       91      +91     
- Partials         0       13      +13     
Impacted Files Coverage Δ
apprise/Apprise.py 91.89% <100.00%> (-8.11%) ⬇️
apprise/py3compat/asyncio.py 100.00% <100.00%> (ø)
apprise/config/__init__.py 79.16% <0.00%> (-20.84%) ⬇️
apprise/conversion.py 91.78% <0.00%> (-8.22%) ⬇️
apprise/plugins/__init__.py 93.82% <0.00%> (-6.18%) ⬇️
apprise/URLBase.py 95.96% <0.00%> (-4.04%) ⬇️
apprise/plugins/NotifyFCM/color.py 96.66% <0.00%> (-3.34%) ⬇️
apprise/plugins/NotifySES.py 97.14% <0.00%> (-2.86%) ⬇️
apprise/plugins/NotifyFCM/priority.py 97.14% <0.00%> (-2.86%) ⬇️
... and 14 more

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 113fa36...314b5e6. Read the comment docs.

@caronc
Copy link
Owner

caronc commented Jul 9, 2022

Thanks for the PR; looks like your code just doesn't handle Py36. Not sure if this merge request is even required after your great alternative solution you provided.

Is it worth adding this extra if/else section of code? Thoughts

@YoRyan
Copy link
Contributor Author

YoRyan commented Jul 10, 2022

Is it worth adding this extra if/else section of code? Thoughts

Well, it is a breakage - code that could previously call notify() from an async loop will no longer work. I suppose the merge/no merge question depends on how important you consider this kind of scenario.

@caronc
Copy link
Owner

caronc commented Jul 10, 2022

Thanks for you quick reply. Your async knowledge is always so impressive. Would there be much needed to be compatible with py36?

@YoRyan
Copy link
Contributor Author

YoRyan commented Jul 12, 2022

Fixed! The test shouldn't have run with py36 in the first place, because it doesn't support asyncio.run().

(It's still not passing Travis because an unrelated test is failing. Looks like an external resource on outlook.com was not available.)

@caronc
Copy link
Owner

caronc commented Jul 13, 2022

Thank you very much for all your great work!

@caronc caronc merged commit b34051c into caronc:master Jul 13, 2022
@YoRyan YoRyan deleted the fix-nested-async branch July 16, 2022 00:20
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