[pushbullet] Pushbullet Binding initial contribution #5668

hakan42 · 2019-05-31T21:16:02Z

This is a reimplementation of the pushbullet action which I had authored for openHAB 1:

https://www.openhab.org/addons/actions/pushbullet/

Most of the "business" code is transplanted from OH1, and some adaptations made to be able to use this as an annotated action.

I have not tried to configure the rule via Paper UI, though.

openhab-bot · 2019-05-31T21:25:48Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/pushbullet-re-implemented-for-openhab-2/75543/1

bundles/org.openhab.binding.pushbullet/.classpath

davidgraeff · 2019-05-31T21:34:51Z

You need to add this bundle to the bill of materials (bom/..) file as well :)

hakan42 · 2019-06-01T08:36:53Z

rebuild

hakan42 · 2019-06-02T07:58:40Z

rebuild...

hakan42 · 2019-06-02T08:14:38Z

Build failures are not caused by the binding itself.

See https://github.com/openhab/openhab2-addons/pull/5582

wborn

Thanks for porting the action to an OH2 binding! I've added a few comments below:

...nding.pushbullet/src/main/java/org/openhab/binding/pushbullet/handler/PushbulletHandler.java

...hab.binding.pushbullet/src/main/java/org/openhab/binding/pushbullet/internal/model/Push.java

...inding.pushbullet/src/main/java/org/openhab/binding/pushbullet/internal/model/PushError.java

...ing.pushbullet/src/main/java/org/openhab/binding/pushbullet/internal/model/PushResponse.java

bundles/org.openhab.binding.pushbullet/.project

wborn · 2019-06-03T23:50:54Z

...ding.pushbullet/src/main/java/org/openhab/binding/pushbullet/PushbulletBindingConstants.java

+ *
+ * SPDX-License-Identifier: EPL-2.0
+ */
+package org.openhab.binding.pushbullet;


Binding classes should no longer be exported by defaults.
So if possible move all classes into the internal package.

Suggested change

package org.openhab.binding.pushbullet;

package org.openhab.binding.internal.pushbullet;

Would that be all classes like handlers and action definitions and so on? Or just the constants? Could you point me to a known-to-be-good example bundle?

All classes. Constants are actually the exception, if they are used across multiple bundles (like with the MQTT main and sub bundles).

It should be:

Suggested change

package org.openhab.binding.pushbullet;

package org.openhab.binding.pushbullet.internal;

The internal must be in the specific binding package not the other way around.

That's right! 👍

o.k., done.

...nding.pushbullet/src/main/java/org/openhab/binding/pushbullet/handler/PushbulletHandler.java

hakan42 · 2019-06-13T07:30:43Z

@wborn , your suggested changes lead to DCO violations.

Do you want me to squash the commits and force-push or will you manually "accept" them? Also, is there a way to improve our DCO checker for those "Co-Authored-By" lines?

wborn · 2019-06-13T08:10:21Z

I'm OK with squashing all the commits @hakan42 to fix the sign offs.

It's not strictly necessary because we can also fix the sign offs when we squash them when merging this PR.

DCO failing on those commits seems to be a known issue: dcoapp/app#102.

Also note that we use an Also-by: line for additional authors instead of Co-Authored-By:, see Sign your work.

If there are still pending or ongoing reviews it's usually better not to squash the commits so reviewers can check the changes.

hakan42 · 2019-06-13T08:10:42Z

@wborn , @davidgraeff , I believe I adapted all your comments. Could you please give the PR another look? I'd love to have it merged for Milestone 2 😄

hakan42 · 2019-06-13T16:18:30Z

Jenkins, please rebuild...

hakan42 · 2019-06-21T11:41:22Z

Jenkins, please rebuild...

hakan42 · 2019-06-29T13:01:29Z

@openhab/2-x-add-ons-maintainers , could I ask for a friendly "LGTM"? This binding is working in my productive environment for multiple weeks now.

I squashed the commits to get rid of a wrong commit email address (incorrectly used the work git configuration), and rebased against the current master...

Signed-off-by: Hakan Tandogan <hakan@tandogan.com>

martinvw

Thanks for your binding I added some remarks, especially the catching of the Error is a no-go for me. Could you elaborate on that?

bundles/org.openhab.binding.pushbullet/README.md

...hbullet/src/main/java/org/openhab/binding/pushbullet/internal/handler/PushbulletHandler.java

martinvw · 2019-06-30T12:41:37Z

...hbullet/src/main/java/org/openhab/binding/pushbullet/internal/handler/PushbulletHandler.java

+
+        logger.debug("sendPush is called for ");
+        logger.debug("Thing {}", thing);
+        logger.debug("Thing Label: '{}'", thing.getLabel());


Wouldn't this be resolved by the toString of the argument of the above logline

In principle, yes. I just like being showed line-by-line debug information. Consider it a matter of taste please :)

martinvw · 2019-06-30T12:41:43Z

...hbullet/src/main/java/org/openhab/binding/pushbullet/internal/handler/PushbulletHandler.java

+
+        PushbulletConfiguration configuration = getConfigAs(PushbulletConfiguration.class);
+        logger.debug("CFG {}", configuration);
+        logger.debug("CFG Name  '{}'", configuration.getName());


Wouldn't this be resolved by the toString of the argument of the above logline

In principle, yes. I just like being showed line-by-line debug information. Consider it a matter of taste please :)

...hbullet/src/main/java/org/openhab/binding/pushbullet/internal/handler/PushbulletHandler.java

martinvw · 2019-06-30T12:42:54Z

...hbullet/src/main/java/org/openhab/binding/pushbullet/internal/handler/PushbulletHandler.java

+                logger.warn("IO problems pushing note: {}", e.getMessage());
+            }
+
+        } catch (java.lang.Error e) {


Do not catch Error

In this case, I have to catch Error to be able to obtain information about the HttpResponseException. Thereby, I can inform the user about expired tokens.

I could check for HttpResponseException, and if that is not the case, rethrow the original error. Would that be o.k. for you?

But who Throws an error, please not that errors are different branch from Exception it contains things like OutOfMemmoryError, StackOverflow etc

What exact error do you intend to catch and who threw it?

Someone deep inside HttpUtil.executeUrl()... I'll try to provoke an exception so I can provide you with a full stack trace.

In the mean time, I'll check for the HttpResponseException (as mentioned above), and rethrow the exception if it did not contain what I was looking for. Would that be o.k. for you?

Someone deep inside HttpUtil.executeUrl()... I'll try to provoke an exception so I can provide you with a full stack trace.

In the mean time, I'll check for the HttpResponseException (as mentioned above), and rethrow the exception if it did not contain what I was looking for. Would that be o.k. for you?

Exception would be 'fine' (and are indeed throw from HttpUtil) but you are catching the Error.

Please provide me with it, I would be surprised if its not an Exception but really an Error

Please provide me with it, I would be surprised if its not an Exception but really an Error

As luck would have it, I am getting "normal" JSON answers to an invalid token just now. Whereas, earlier, it was packaged in the Error. Maybe the API changed since I had written the 1.x action class.

I'll remove the complete error-catching part and wait whether it happens again, then we can talk about how to handle it.

I'll remove the complete error-catching part and wait whether it happens again, then we can talk about how to handle it.

Great, just ping me when you are ready, we should be close to a merge then

@martinvw , I did refactor as discussed, and Travis is happy too. I think Jenkins will also be whenever it gets around to build this branch.

Could you give the PushbulletHandler class another look please?

martinvw · 2019-06-30T12:43:24Z

...hbullet/src/main/java/org/openhab/binding/pushbullet/internal/handler/PushbulletHandler.java

+            logger.debug("Error in getResponseString: ", e);
+        }
+
+        return result;


Consider splitting up this method

But how? logically, it is one operation. splitting it up would only serve to introduce multiple methods, but to no gain with regard to the logic that has to be performed.

I would say that it contains a lot of logic units, eg contructing the push message, or constructing and convert it to a string. Or performing the actual request.

I'll refactor it later tonight, at the same time working on the error-catching part.

martinvw · 2019-06-30T12:45:19Z

bom/openhab-addons/pom.xml

@@ -600,6 +600,11 @@
      <artifactId>org.openhab.binding.pulseaudio</artifactId>
      <version>${project.version}</version>
    </dependency>
+    <dependency>


@J-N-K / @maggu2810 / @openhab/2-x-add-ons-maintainers should a binding contain a feature.xml file now or only in special cases?

I did not follow the "a feature per addon" changes.
Perhaps this could be answered by @openhab/2-x-add-ons-maintainers

As far as I can see, all bindings contain a feature.xml (looking at the HEAD of openhab2-addons repo). Therefore, I moved "my" feature.xml into the binding source tree as well.

Signed-off-by: Hakan Tandogan <hakan@tandogan.com>

hakan42 · 2019-07-01T07:30:39Z

By the way, Jenkins failure is caused by an Out of Memory error while doing the karaf-feature-verification. Not directly caused by this binding, I hope :)

…ror anymore Signed-off-by: Hakan Tandogan <hakan@tandogan.com>

hakan42 · 2019-07-03T13:35:14Z

Jenkins, please rebuild

martinvw · 2019-07-03T18:24:38Z

bundles/pom.xml

@@ -156,6 +156,7 @@
    <module>org.openhab.binding.plugwise</module>
    <module>org.openhab.binding.powermax</module>
    <module>org.openhab.binding.pulseaudio</module>
+    <module>org.openhab.binding.pushbullet</module>


Could you add the binding to the codeowners

The first file in the "changes" tab shows the addition to CODEOWNERS.

I'll wait for your merge, and if does not show up in the HEAD , I'll create an additional PR as suggested

martinvw · 2019-07-03T18:26:28Z

...hbullet/src/main/java/org/openhab/binding/pushbullet/internal/handler/PushbulletHandler.java

+            if ((null != response) && (null == response.getPushError())) {
+                result = true;
+            }
+        }


Formatting is a little bit a-typical

Maybe caused by my IntelliJ installation. It should have applied the Eclipse formatting rules though.

Can I fix that in an upcoming PR when I touch this code again?

martinvw

Except for the missing codeowners its fine, feel free to create another PR for that. If no-one else has something before 22:00 CEST I will merge.

davidgraeff

Looks good except those multiple debug lines (remember that each log line is executed even if not at the debug level. Meaning loading the arguments to registers, initiate a call stack, jump to the method, call the framework to determine the logger level we are in, aborting)

martinvw · 2019-07-03T20:19:42Z

Looks good except those multiple debug lines (remember that each log line is executed even if not at the debug level. Meaning loading the arguments to registers, initiate a call stack, jump to the method, call the framework to determine the logger level we are in, aborting)

Agree on that, it would be nice if you be logging less next time or combine it in a lot less statements with the proper placeholders etc.

hakan42 · 2019-07-04T07:11:54Z

Thank you so much guys. And off I go, working on the next binding :)

Signed-off-by: Hakan Tandogan <hakan@tandogan.com>

Signed-off-by: Hakan Tandogan <hakan@tandogan.com> Signed-off-by: Maximilian Hess <mail@ne0h.de>

Signed-off-by: Hakan Tandogan <hakan@tandogan.com>

Signed-off-by: Hakan Tandogan <hakan@tandogan.com> Signed-off-by: Tim Roberts <timmarkroberts@gmail.com>

hakan42 requested a review from a team as a code owner May 31, 2019 21:16

wborn added work in progress A PR that is not yet ready to be merged new binding If someone has started to work on a binding. For a new binding PR. labels May 31, 2019

hakan42 added the binding: pushbullet label May 31, 2019

davidgraeff reviewed May 31, 2019

View reviewed changes

bundles/org.openhab.binding.pushbullet/.classpath Outdated Show resolved Hide resolved

hakan42 closed this Jun 1, 2019

hakan42 reopened this Jun 1, 2019

hakan42 changed the title ~~[WIP] [pushbullet] Pushbullet binding, reimplemented for openHAB 2~~ [pushbullet] Pushbullet binding, reimplemented for openHAB 2 Jun 2, 2019

hakan42 closed this Jun 2, 2019

hakan42 reopened this Jun 2, 2019

wborn added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build work in progress A PR that is not yet ready to be merged labels Jun 3, 2019

wborn changed the title ~~[pushbullet] Pushbullet binding, reimplemented for openHAB 2~~ [pushbullet] Pushbullet Binding initial contribution Jun 3, 2019

wborn reviewed Jun 6, 2019

View reviewed changes

hakan42 closed this Jun 13, 2019

hakan42 reopened this Jun 13, 2019

hakan42 closed this Jun 21, 2019

hakan42 reopened this Jun 21, 2019

hakan42 force-pushed the pushbullet-binding branch from 0df3f93 to b9cba02 Compare June 21, 2019 11:45

hakan42 force-pushed the pushbullet-binding branch from d8e5933 to d80050a Compare June 29, 2019 12:36

This was referenced Jun 29, 2019

[pushbullet] move action to legacy openhab/openhab1-addons#5874

Merged

[pushbullet] add note about retired pushbullet action openhab/openhab-distro#934

Merged

[pushbullet] reimplementation for openHAB2

deeef36

Signed-off-by: Hakan Tandogan <hakan@tandogan.com>

hakan42 force-pushed the pushbullet-binding branch from d80050a to deeef36 Compare June 29, 2019 13:25

martinvw reviewed Jun 30, 2019

View reviewed changes

hakan42 added 3 commits June 30, 2019 21:31

[pushbullet] removed superfluous newlines

568471e

Signed-off-by: Hakan Tandogan <hakan@tandogan.com>

[pushbullet] remove superfluous state change

06eb671

Signed-off-by: Hakan Tandogan <hakan@tandogan.com>

[pushbullet] remove superfluous toString call

74a35a8

Signed-off-by: Hakan Tandogan <hakan@tandogan.com>

hakan42 force-pushed the pushbullet-binding branch from 1fa8067 to 74a35a8 Compare June 30, 2019 22:45

[pushbullet] Split up sendPush() into smaller methods, don't catch Er…

19196eb

…ror anymore Signed-off-by: Hakan Tandogan <hakan@tandogan.com>

hakan42 closed this Jul 3, 2019

hakan42 reopened this Jul 3, 2019

martinvw reviewed Jul 3, 2019

View reviewed changes

martinvw approved these changes Jul 3, 2019

View reviewed changes

martinvw requested a review from a team July 3, 2019 18:27

davidgraeff approved these changes Jul 3, 2019

View reviewed changes

martinvw merged commit 86191c5 into openhab:master Jul 3, 2019

wborn added this to the 2.5 milestone Jul 30, 2019

sprehn pushed a commit to sprehn/openhab-addons that referenced this pull request Aug 21, 2019

[pushbullet] Pushbullet Binding initial contribution (openhab#5668)

3e6ffd7

Signed-off-by: Hakan Tandogan <hakan@tandogan.com>

jsetton mentioned this pull request Aug 26, 2019

[pushbullet] Backward 1.x compatibility issue with broadcast notifications #5982

Closed

ne0h pushed a commit to ne0h/openhab-addons that referenced this pull request Sep 15, 2019

[pushbullet] Pushbullet Binding initial contribution (openhab#5668)

5dbe291

Signed-off-by: Hakan Tandogan <hakan@tandogan.com> Signed-off-by: Maximilian Hess <mail@ne0h.de>

Pshatsillo pushed a commit to Pshatsillo/openhab-addons that referenced this pull request Dec 8, 2019

[pushbullet] Pushbullet Binding initial contribution (openhab#5668)

8be6b70

Signed-off-by: Hakan Tandogan <hakan@tandogan.com>

tmrobert8 pushed a commit to tmrobert8/openhab-addons that referenced this pull request Jan 21, 2020

[pushbullet] Pushbullet Binding initial contribution (openhab#5668)

0bdc2cd

Signed-off-by: Hakan Tandogan <hakan@tandogan.com> Signed-off-by: Tim Roberts <timmarkroberts@gmail.com>

	package org.openhab.binding.pushbullet;
	package org.openhab.binding.internal.pushbullet;

	package org.openhab.binding.pushbullet;
	package org.openhab.binding.pushbullet.internal;

[pushbullet] Pushbullet Binding initial contribution #5668

[pushbullet] Pushbullet Binding initial contribution #5668

Conversation

hakan42 commented May 31, 2019

openhab-bot commented May 31, 2019

davidgraeff commented May 31, 2019

hakan42 commented Jun 1, 2019

hakan42 commented Jun 2, 2019

hakan42 commented Jun 2, 2019

wborn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakan42 commented Jun 13, 2019

wborn commented Jun 13, 2019

hakan42 commented Jun 13, 2019

hakan42 commented Jun 13, 2019

hakan42 commented Jun 21, 2019

hakan42 commented Jun 29, 2019

martinvw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakan42 Jun 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinvw Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakan42 commented Jul 1, 2019

hakan42 commented Jul 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinvw left a comment

Choose a reason for hiding this comment

davidgraeff left a comment

Choose a reason for hiding this comment

martinvw commented Jul 3, 2019

hakan42 commented Jul 4, 2019

wborn left a comment •

edited

Loading

hakan42 Jun 30, 2019 •

edited

Loading

martinvw Jul 1, 2019 •

edited

Loading