-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[openhabcloud] Support hiding notifications and tags #16979
Conversation
See openhab#16934 Signed-off-by: Dan Cunningham <dan@digitaldan.com>
Signed-off-by: Dan Cunningham <dan@digitaldan.com>
This adds notification support as well as renames |
Signed-off-by: Dan Cunningham <dan@digitaldan.com>
Ok, ready for review |
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.
Overall LGTM, pleas have a look at my comments.
...rg.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/NotificationAction.java
Outdated
Show resolved
Hide resolved
...rg.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/NotificationAction.java
Outdated
Show resolved
Hide resolved
...rg.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/NotificationAction.java
Outdated
Show resolved
Hide resolved
...rg.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/NotificationAction.java
Outdated
Show resolved
Hide resolved
....openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/internal/CloudClient.java
Outdated
Show resolved
Hide resolved
...rg.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/NotificationAction.java
Outdated
Show resolved
Hide resolved
...rg.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/NotificationAction.java
Outdated
Show resolved
Hide resolved
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.
formally
-> formerly
...rg.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/NotificationAction.java
Outdated
Show resolved
Hide resolved
Depends on openhab#16979. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
I have just published openhab-js 5.3.1 for this, and provided an add-on PR: #16985 |
I guess this should be classified as enhancement and needs to wait before 4.3 is release? |
I would like to get this into 4.2 - I’d consider this PR a similar situation to merging new bindings. If this is not merged for 4.2.0, we either need another extension which gives us more and more method overrides for the actions, or we need to do a breaking change in 4.3.0. |
+1 for including it into 4.2. The enhanced notification had already made it in between M3 and M4. If we didin't include this PR, we'd have a method in 4.1 with 4 arguments, 4.2 with enhanced but without reference id, and 4.3 with reference id, increasing the number of different versions of the API across OH versions that we'd have to support in the helper library. So we should consider this a part of the "enhanced notification feature" that had already made the cut into M4. |
Oh yes, indeed if this was postponed into 4.3.0 it would be a breaking change :( |
PS, I'll update the jruby examples in the README after this PR had been merged and the JRuby changes to support it also merged (which depends on this PR). |
Signed-off-by: Dan Cunningham <dan@digitaldan.com>
Thanks for the feedback! Changes pushed. I agree we should get this into 4.2, its important we try and get these method signatures right in one shot, otherwise we are going to prematurely introduce more complexity into the binding that is not needed yet. |
@lsiepel This is ready to review when you are :-) |
Able to review Thursday evening, not sooner. If another maintainer is available, please proceed. |
@digitaldan Some of my review comments are still open, can you please take a look? |
Ah, sorry i missed those, github was hiding them! |
Signed-off-by: Dan Cunningham <dan@digitaldan.com>
Signed-off-by: Dan Cunningham <dan@digitaldan.com>
Thanks @florian-h05 , hopefully i got them all this time. |
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.
LGTM now, thanks!
Just one minor comment:
Done! Thanks! |
Depends on openhab#16979. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Depends on openhab#16979. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
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.
Many thanks for these improvements!
Depends on #16979. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Thanks 🙌 |
* Support hiding notifications See openhab#16934 * Adds support for using tag over severity on extended actions Signed-off-by: Dan Cunningham <dan@digitaldan.com>
Depends on openhab#16979. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
* Support hiding notifications See openhab#16934 * Adds support for using tag over severity on extended actions Signed-off-by: Dan Cunningham <dan@digitaldan.com> Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
Depends on openhab#16979. Signed-off-by: Florian Hotze <florianh_dev@icloud.com> Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
* Support hiding notifications See openhab#16934 * Adds support for using tag over severity on extended actions Signed-off-by: Dan Cunningham <dan@digitaldan.com>
Depends on openhab#16979. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
* Support hiding notifications See openhab#16934 * Adds support for using tag over severity on extended actions Signed-off-by: Dan Cunningham <dan@digitaldan.com>
Depends on openhab#16979. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
See #16934