Add support for recent CPython and PyPy versions, drop support for Python 2 #680
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,15 +26,6 @@ sdist/ | |
| .installed.cfg | ||
| *.egg | ||
|
|
||
| # Installer logs | ||
| pip-log.txt | ||
| pip-delete-this-directory.txt | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ | |
| # THE SOFTWARE. | ||
|
|
||
| import os | ||
| import warnings | ||
| from itertools import chain | ||
| from . import common | ||
| from .conversion import convert_between | ||
|
|
@@ -44,13 +44,13 @@ | |
| from . import plugins | ||
| from . import __version__ | ||
|
|
||
| # Python v3+ support code made importable, so it can remain backwards | ||
| # compatible with Python v2 | ||
| # TODO: Review after dropping support for Python 2. | ||
| from . import py3compat | ||
|
There was a problem hiding this comment. I think the There was a problem hiding this comment. I agree; this definitely needs to be revisited... a lot of @YoRyan's work with creating the I think it's great the extra CI tasks you added |
||
|
|
||
|
|
||
| class Apprise: | ||
| """ | ||
| Our Notification Manager | ||
|
|
||
|
|
@@ -124,7 +124,7 @@ def instantiate(url, asset=None, tag=None, suppress_exceptions=True): | |
| # Prepare our Asset Object | ||
| asset = asset if isinstance(asset, AppriseAsset) else AppriseAsset() | ||
|
|
||
| if isinstance(url, str): | ||
caronc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| # Acquire our url tokens | ||
| results = plugins.url_to_dict( | ||
| url, secure_logging=asset.secure_logging) | ||
|
|
@@ -247,7 +247,7 @@ def add(self, servers, asset=None, tag=None): | |
| # prepare default asset | ||
| asset = self.asset | ||
|
|
||
| if isinstance(servers, str): | ||
| # build our server list | ||
| servers = parse_urls(servers) | ||
| if len(servers) == 0: | ||
|
|
@@ -275,7 +275,7 @@ def add(self, servers, asset=None, tag=None): | |
| self.servers.append(_server) | ||
| continue | ||
|
|
||
| elif not isinstance(_server, (str, dict)): | ||
| logger.error( | ||
| "An invalid notification (type={}) was specified.".format( | ||
| type(_server))) | ||
|
|
@@ -306,7 +306,7 @@ def clear(self): | |
|
|
||
| def find(self, tag=common.MATCH_ALL_TAG, match_always=True): | ||
| """ | ||
| Returns a list of all servers matching against the tag specified. | ||
|
|
||
| """ | ||
|
|
||
|
|
@@ -347,14 +347,14 @@ def notify(self, body, title='', notify_type=common.NotifyType.INFO, | |
| body_format=None, tag=common.MATCH_ALL_TAG, match_always=True, | ||
| attach=None, interpret_escapes=None): | ||
| """ | ||
| Send a notification to all the plugins previously loaded. | ||
|
|
||
| If the body_format specified is NotifyFormat.MARKDOWN, it will | ||
| be converted to HTML if the Notification type expects this. | ||
|
|
||
| if the tag is specified (either a string or a set/list/tuple | ||
| of strings), then only the notifications flagged with that | ||
| tagged value are notified. By default, all added services | ||
| are notified (tag=MATCH_ALL_TAG) | ||
|
|
||
| This function returns True if all notifications were successfully | ||
|
|
@@ -363,59 +363,37 @@ def notify(self, body, title='', notify_type=common.NotifyType.INFO, | |
| simply having empty configuration files that were read. | ||
|
|
||
| Attach can contain a list of attachment URLs. attach can also be | ||
| represented by an AttachBase() (or list of) object(s). This | ||
| identifies the products you wish to notify | ||
|
|
||
| Set interpret_escapes to True if you want to pre-escape a string | ||
| such as turning a \n into an actual new line, etc. | ||
| """ | ||
|
|
||
| return py3compat.asyncio.tosync( | ||
| self.async_notify( | ||
| body, title, | ||
| notify_type=notify_type, body_format=body_format, | ||
| tag=tag, match_always=match_always, attach=attach, | ||
| interpret_escapes=interpret_escapes, | ||
| ), | ||
| debug=self.debug | ||
| ) | ||
|
|
||
| def async_notify(self, *args, **kwargs): | ||
|
There was a problem hiding this comment. The docstring of the
After dropping support for Python 2, the There was a problem hiding this comment. Yes, you can add There was a problem hiding this comment. Let's not get hasty on this one... a lot of people have adopted There was a problem hiding this comment. No need to be worried; there is no backwards compatibility issue. async def async_notify(self, *args, **kwargs)just makes it a little more obvious that this method must be called with There was a problem hiding this comment. Indeed, if you look at the corresponding .pyi file, you'll see that the type annotation already has the There was a problem hiding this comment. Also pleased to meet you, Andreas!
The function already returns a future, hence every return value being wrapped with There was a problem hiding this comment.
Aww, thanks. 😁 Yes, we absolutely can leave this for later, and I would be happy to make that PR. For the record, if you add the def myAsyncFunction():
return py3compat.asyncio.toasyncwrapvalue("Hello, World!")async def myAsyncFunction():
return "Hello, World!"There was a problem hiding this comment. Thanks for providing an example. Let me demonstrate, while the returning behavior is being made equivalent, the invocation behavior, at runtime, is not. The following code, where the On the other hand, when you will prefix that routine with the On that matter, I think Chris is absolutely right:
However, it will not break because of not waiting for the completion of the function's future, that would only be about getting a return value anyway, and making sure it completed. It will break for all people who either unaware or errorneously are invoking the function synchronously, and then suddenly finding it would stop being executed, which might be many. All users who are doing it this way, must change their code to I think this would be a change which should only be done on a major release bump, designated as "breaking change". import apprise
def async_foo():
raise ValueError("foo")
return apprise.py3compat.asyncio.toasyncwrapvalue("Hello, World!")
def main():
async_foo()
if __name__ == "__main__":
main()There was a problem hiding this comment. Oops, I understand now. I had not considered synchronous callers who would not be interested in the return value and who might inadvertently discard the future. I absolutely intended for async_notify() to behave just like an But in contrast to "a lot of people," I doubt this breakage would affect that many at all. The new async method was only introduced fairly recently, and the "async_" prefix combined with the type annotation should have served as reminders that the returned future needs to be await'ed. Nevertheless, yes, it is a breakage, and I agree it should be documented for a future PR and release. There was a problem hiding this comment. Hi again,
It is still 1.5 years, though ;].
Sure, I think I might have exeggarated this a bit. I only wanted to outline the potential breaking change with insistence, but not overshoot the discussion. It may have happened - apologies for that.
I think Chris will be happy if you continue throwing in your expertise on this matter. As we observed yesterday with a testing build on Copr, just slapping an Whether the improvement may be released within the 1.x series, with a corresponding admonition in the changelog, or just right away with a new 2.x release, is of course up to you. My verdict is: Why not just make a 2.x when the switchover is ready - I think it will not do any harm whatsoever. Keep up the spirit, thanks for all the excellent discussions, and with kind regards, |
||
| """ | ||
| Send a notification to all the plugins previously loaded, for | ||
| asynchronous callers. This method is an async method that should be | ||
| awaited on, even if it is missing the async keyword in its signature. | ||
| (This is omitted to preserve syntax compatibility with Python 2.) | ||
|
|
||
| The arguments are identical to those of Apprise.notify(). | ||
|
|
||
| # TODO: Add `async` keyword. | ||
| """ | ||
| warnings.warn("A future version of Apprise will add the `async` " | ||
|
There was a problem hiding this comment. The We don't need this warning at all. We just didn't have the There was a problem hiding this comment. Maybe I am not right on track yet, but didn't we just observe that adding Maybe it is too late at night already for me to correctly understand why adding There was a problem hiding this comment. You might be right here; but to my knowledge we were |
||
| "keyword to `async_notify`. Please make sure your code " | ||
| "will correctly `await` the returned future.") | ||
|
|
||
| try: | ||
| coroutines = list( | ||
|
|
@@ -477,7 +455,7 @@ def _notifyall(self, handler, body, title='', | |
| tag=common.MATCH_ALL_TAG, match_always=True, attach=None, | ||
| interpret_escapes=None): | ||
| """ | ||
| Creates notifications for all the plugins loaded. | ||
|
|
||
| Returns a generator that calls handler for each notification. The first | ||
| and only argument supplied to handler is the server, and the keyword | ||
|
|
@@ -496,23 +474,11 @@ def _notifyall(self, handler, body, title='', | |
| raise TypeError(msg) | ||
|
|
||
| try: | ||
| if title and isinstance(title, bytes): | ||
| title = title.decode(self.asset.encoding) | ||
|
|
||
| if body and isinstance(body, bytes): | ||
| body = body.decode(self.asset.encoding) | ||
|
|
||
| except UnicodeDecodeError: | ||
| msg = 'The content passed into Apprise was not of encoding ' \ | ||
|
|
@@ -580,43 +546,12 @@ def _notifyall(self, handler, body, title='', | |
| .encode('ascii', 'backslashreplace')\ | ||
| .decode('unicode-escape') | ||
|
|
||
amotl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| except AttributeError: | ||
| # Must be of string type | ||
| msg = 'Failed to escape message body' | ||
| logger.error(msg) | ||
| raise TypeError(msg) | ||
|
|
||
| yield handler( | ||
| server, | ||
| body=conversion_body_map[server.notify_format], | ||
|
|
@@ -669,12 +604,12 @@ def details(self, lang=None, show_requirements=False, show_disabled=False): | |
|
|
||
| # Standard protocol(s) should be None or a tuple | ||
| protocols = getattr(plugin, 'protocol', None) | ||
| if isinstance(protocols, str): | ||
| protocols = (protocols, ) | ||
|
|
||
| # Secure protocol(s) should be None or a tuple | ||
| secure_protocols = getattr(plugin, 'secure_protocol', None) | ||
| if isinstance(secure_protocols, str): | ||
| secure_protocols = (secure_protocols, ) | ||
|
|
||
| # Add our protocol details to our content | ||
|
|
@@ -779,15 +714,8 @@ def __getitem__(self, index): | |
|
|
||
| def __bool__(self): | ||
| """ | ||
| Allows the Apprise object to be wrapped in an 'if statement'. | ||
| True is returned if at least one service has been loaded. | ||
| """ | ||
| return len(self) > 0 | ||
|
|
||
|
|
@@ -807,7 +735,3 @@ def __len__(self): | |
| """ | ||
| return sum([1 if not isinstance(s, (ConfigBase, AppriseConfig)) | ||
| else len(s.servers()) for s in self.servers]) | ||
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.
Any value in bringing this back? This 1 liner allowed all pip calls to install cryptography (regardless of what version)
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 right there, just tweaked it a bit and accidentally removed your comment.