added ntfy notifier #289
added ntfy notifier #289
Conversation
Bump version 1.15.2
|
Hi @dalins. |
Der-Henning
left a comment
Der-Henning
left a comment
There was a problem hiding this comment.
Thank you for your contribution. I don't have any urgent change requests, but some points to discuss.
src/notifiers/ntfy.py
Outdated
| log.debug("Sending ntfy Notification") | ||
|
|
||
| url = f"{self.server}/{self.topic}" | ||
| log.debug("ntfy url: %s", url) |
There was a problem hiding this comment.
As the url does not change on every consecutive call, logging it doesn't add any meaningful information. Consider logging the body as it contains variables.
There was a problem hiding this comment.
Okay, I think it would be nice to see the final url and therefore I move it into the __init__ method. Are you fine with this solution?
| log = logging.getLogger('tgtg') | ||
|
|
||
|
|
||
| class Ntfy(Notifier): |
There was a problem hiding this comment.
The notifier could also be based on the Webhook notifier like the IFTTT notifier. It would only be necessary to add the auth property to the Webhook notifier, which would in general be a good addition.
This would centralize the request logic and make maintaining the code easier.
Using self.__class__.__name__ can be used in the Webhook logs to display the calling Notifier.
There was a problem hiding this comment.
Good idea, but I think adding authentication to the WebHook should be an other pull request, because it is not part of this feature and affects WebHook and IFTTT functionality. But I like the idea to centralize the request logic and will do that. So are you fine with this argumentation?
Co-authored-by: Henning Merklinger <[email protected]>
|
The latest edge docker images and this pre-Release now contain the ntfy notifier. I will create a new Release after some testing. |
2 participants
No description provided.