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

[satel] Action for reading the event log added #7282

Merged
merged 1 commit into from
Apr 5, 2020

Conversation

druciak
Copy link
Member

@druciak druciak commented Apr 4, 2020

This PR add a rule action for reading records from the event log. It replaces current way of reading the log using items linked to channels of event-log thing. This is still available but marked as deprecated.

It also contains minor changes in README file.

Signed-off-by: Krzysztof Goworek [email protected]

@druciak druciak added the enhancement An enhancement or new feature for an existing add-on label Apr 4, 2020
@druciak druciak requested review from Hilbrand, mhilbush and J-N-K April 4, 2020 12:42
@TravisBuddy
Copy link

Travis tests were successful

Hey @druciak,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

eventIdx = eventRec.get("prev_index")
]
logInfo("EventLog", "End")
getActions("mail","mail:smtp:local").sendMail("[email protected]", "Event log", msgBody)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a note seomwhere that this requires the mail binding.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just an example. The purpose of the action is to read single record from the event log. It does not specify what one should do with the result.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I was just thinking that someone installs satel, adds this example and runs into an error. Maybe a comment in the line above this one like sending notifications via mail requires the mail binding or something like that would be good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

/**
* Represents single record of the event log.
*
* @author kgoworek
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a real name here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@TravisBuddy
Copy link

Travis tests were successful

Hey @druciak,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@@ -55,6 +55,8 @@ Example:
Bridge satel:ethm-1:home [ host="192.168.0.2", refresh=1000, userCode="1234", encryptionKey="abcdefgh" ]
```

**NOTE:** There can be only one client connected to ETHM-1 module. It does not accept new connections if there is already a connection established. In case you have troubles connecting to the system using this module, please make sure there is no other client (for example installed 1.x version of the binding) already connected to it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a new line for each sentence. They are concatenated automatically.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


### readEvent

This action allows you to read one record from the event log placed at index given by input parameter. The result of this action is compatible with channels of `event-log` thing and contains following values:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

eventIdx = eventRec.get("prev_index")
]
logInfo("EventLog", "End")
getActions("mail","mail:smtp:local").sendMail("[email protected]", "Event log", msgBody)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I was just thinking that someone installs satel, adds this example and runs into an error. Maybe a comment in the line above this one like sending notifications via mail requires the mail binding or something like that would be good.

@TravisBuddy
Copy link

Travis tests were successful

Hey @druciak,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@druciak
Copy link
Member Author

druciak commented Apr 5, 2020

@J-N-K I think I resolved all your comments. Could you please look at the code if there's still something to fix?

Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@J-N-K J-N-K merged commit 3b49fb9 into openhab:2.5.x Apr 5, 2020
@cpmeister cpmeister added this to the 2.5.4 milestone Apr 5, 2020
@druciak
Copy link
Member Author

druciak commented Apr 5, 2020

Thanks for merging this PR so quickly.

Please add the breaking change to this file: https://github.com/openhab/openhab-distro/blob/master/distributions/openhab/src/main/resources/bin/update.lst

The old way is still available, should I add breaking change in such case as well?

cweitkamp pushed a commit to openhab/openhab-distro that referenced this pull request Apr 8, 2020
Hans-Reiner pushed a commit to Hans-Reiner/openhab2-addons that referenced this pull request Apr 11, 2020
Signed-off-by: Krzysztof Goworek <[email protected]>
Signed-off-by: Hans-Reiner Hoffmann <[email protected]>
yfre pushed a commit to yfre/openhab-addons that referenced this pull request Apr 27, 2020
Signed-off-by: Krzysztof Goworek <[email protected]>
Signed-off-by: Eugen Freiter <[email protected]>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request May 29, 2020
LoungeFlyZ pushed a commit to LoungeFlyZ/openhab2-addons that referenced this pull request Jun 8, 2020
J-N-K pushed a commit to J-N-K/openhab-addons that referenced this pull request Jul 14, 2020
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Jul 26, 2020
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
Signed-off-by: Krzysztof Goworek <[email protected]>
Signed-off-by: Daan Meijer <[email protected]>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants