Adding service for logbook #1020
Conversation
| @@ -53,8 +68,19 @@ def log_entry(hass, name, message, domain=None, entity_id=None): | |||
|
|
|||
| def setup(hass, config): | |||
| """ Listens for download events to download files. """ | |||
| hass.http.register_path('GET', URL_LOGBOOK, _handle_get_logbook) | |||
| # create service handler | |||
| def notify_message(notify_service, call): | |||
There was a problem hiding this comment.
This seems like a very complex way of defining a service. Consider doing something similar to this service
|
Just me being stupid :P |
|
I was thinking about moving logbook to notify... Not sure what you think of that but imo that is where it belong. |
| log_entry(hass, name, message) | ||
|
|
||
| hass.http.register_path('GET', URL_LOGBOOK, _handle_get_logbook) | ||
| hass.services.register(DOMAIN, DOMAIN, log_message) |
There was a problem hiding this comment.
logbook.log might be a better service call, in my opinion.
|
I like this PR the way it is. I agree that this is a great addition, however, I disagree that logbook should be a notifier as the role it plays in HASS is fundamentally very different. The logbook records all the activity in HASS, that is its fundamental purpose. Notifiers are used with automations to send messages to end points. This is a very different purpose. I guess a logbook notifier could be made. Then this service could be pushed off to the notifier. However, I don't really like that idea because then the end user is required to configure another component (the logbook notifier) to add additional functionality to an existing component (the logbook) that could have just as easily been baked into the main component (the logbook). As I'm thinking and typing, I guess the logbook component could initialize the logbook notifier; like the Wink component spawns Wink lights, switches, sensors, etc. However, this still doesn't feel the same because a Wink light entity isn't extending the basic functionality of the Wink component. The Wink component exists for the sole purpose of spawning Wink lights, et al. Whereas, the logbook component has its own purpose and does not gain much from splitting out its functionality into multiple components. I guess what I'm saying is that I think creating a notifier for the logbook would add complexity just for complexity's sake. |
|
@rmkraus Well its not the same as a notification service, it do however fill the same role just that this one records som more stuff. For me at this point logbook not that useful with all changed state messages and if I could I would also turn of "automation triggered" and write that message myself with more humanised messages. |
|
@balloob Its renamed 👍 Im happy if you are |
| def log_message(service): | ||
| """ Handle sending notification message service calls. """ | ||
| message = service.data.get(ATTR_MESSAGE) | ||
| name = service.data.get(ATTR_NAME, None) |
There was a problem hiding this comment.
Name is not optional and should not be None
|
I'm against integrating it as a notify service too, it conflates concerns. About filtering the logbook, we should differentiate the concerns of the reader and the writer. All information gets logged, with good reason, it's the history of the system that it is representing. If there is data you do not want to see it should happen at the reader level. Like views for states. But that's a story for a different day. |
|
I fixed domain and entity. I'm not sure about what I should do with description, don't really know where its used and why (have not looked at that part). I agree about the filtering of logbook in reads rather than writes, and yes that's not what this PR should solve. |
| """ Handle sending notification message service calls. """ | ||
| message = service.data.get(ATTR_MESSAGE) | ||
| name = service.data.get(ATTR_NAME) | ||
| domain = service.data.get(ATTR_DOMAIN, None) |
There was a problem hiding this comment.
None is the default return value if not passed in.
|
Bueno 🐬 |
As discussed in #919