-
-
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
[liquidcheck] Initial contribution #13287
Conversation
Hey guys, I'm just curious if someone is reviewing my binding. |
6ba36f5
to
d88ac78
Compare
Hi @marcelGoerentz, thanks for your contribution. Just wanted to let you know that we currently have a lot of PR's to review (including other new bindings as well), so please expect some additional time until a maintainer will be able to perform a full review. Sorry for the inconvenience. |
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.
Review part 1 of 3
bundles/org.openhab.binding.liquidcheck/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
...les/org.openhab.binding.liquidcheck/src/main/resources/OH-INF/i18n/liquidcheck_en.properties
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.liquidcheck/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
...heck/src/main/java/org/openhab/binding/liquidcheck/internal/LiquidCheckBindingConstants.java
Outdated
Show resolved
Hide resolved
...heck/src/main/java/org/openhab/binding/liquidcheck/internal/LiquidCheckBindingConstants.java
Outdated
Show resolved
Hide resolved
...heck/src/main/java/org/openhab/binding/liquidcheck/internal/LiquidCheckBindingConstants.java
Outdated
Show resolved
Hide resolved
Hey @lolodomo, thank you for your review. I appreciate it very much. |
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.
@lolodomo beat me in finding proper amount of time to go through your PR. 😄 So I will just post old minor comments from some time ago when I was having a brief look.
...heck/src/main/java/org/openhab/binding/liquidcheck/internal/LiquidCheckBindingConstants.java
Outdated
Show resolved
Hide resolved
1357a59
to
09ec60e
Compare
Signed-off-by: Marcel Goerentz <[email protected]>
Feel free to finish the review and merge without waiting for me. |
I have not fully reviewed, because I saw you already did. Just wanted to check if there was still something pending from your side preventing a merge, because review comments seemed to have been addressed. |
...dcheck/src/main/java/org/openhab/binding/liquidcheck/internal/LiquidCheckHandlerFactory.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Marcel Goerentz <[email protected]>
@lolodomo - gentle ping. If your review is concluded, please approve. The few comments from my side are resolved, but as said, I haven't reviewed, so would appreciate your confirmation. |
Signed-off-by: Marcel Goerentz <[email protected]>
Signed-off-by: Marcel Goerentz <[email protected]>
Signed-off-by: Marcel Goerentz <[email protected]>
…n response may contain a double. Signed-off-by: marcelGoerentz <[email protected]>
Signed-off-by: marcelGoerentz <[email protected]>
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.
I'm trusting @lolodomo already reviewed this PR. Let's get it finished and merged. I have just a few additional comments after going through changed files.
...ain/java/org/openhab/binding/liquidcheck/internal/discovery/LiquidCheckDiscoveryService.java
Outdated
Show resolved
Hide resolved
...g.liquidcheck/src/main/java/org/openhab/binding/liquidcheck/internal/LiquidCheckHandler.java
Outdated
Show resolved
Hide resolved
...g.liquidcheck/src/main/java/org/openhab/binding/liquidcheck/internal/LiquidCheckHandler.java
Outdated
Show resolved
Hide resolved
...g.liquidcheck/src/main/java/org/openhab/binding/liquidcheck/internal/LiquidCheckHandler.java
Outdated
Show resolved
Hide resolved
…minor changes in handler Signed-off-by: Marcel Goerentz <[email protected]>
Signed-off-by: marcelGoerentz <[email protected]>
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.
Almost there. 🙂
...g.liquidcheck/src/main/java/org/openhab/binding/liquidcheck/internal/LiquidCheckHandler.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/openhab/binding/liquidcheck/internal/httpclient/LiquidCheckHttpClient.java
Outdated
Show resolved
Hide resolved
...g.liquidcheck/src/main/java/org/openhab/binding/liquidcheck/internal/LiquidCheckHandler.java
Outdated
Show resolved
Hide resolved
...g.liquidcheck/src/main/java/org/openhab/binding/liquidcheck/internal/LiquidCheckHandler.java
Outdated
Show resolved
Hide resolved
...g.liquidcheck/src/main/java/org/openhab/binding/liquidcheck/internal/LiquidCheckHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: marcelGoerentz <[email protected]>
Signed-off-by: marcelGoerentz <[email protected]>
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.
Sorry, this fell off my radar. I checked the last two commits and provided two additional comments.
...g.liquidcheck/src/main/java/org/openhab/binding/liquidcheck/internal/LiquidCheckHandler.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/openhab/binding/liquidcheck/internal/httpclient/LiquidCheckHttpClient.java
Outdated
Show resolved
Hide resolved
Signed-off-by: marcelGoerentz <[email protected]>
Signed-off-by: Marcel Goerentz <[email protected]>
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.
Thank you for your contribution and your patience. Let's merge this binding and hopefully get it more widely tested with the next milestone release. If any issues are later found leading to an explanation why you needed to start the shared http client, this can be fixed in a follow-up PR.
Now, you could add your binding's logo to the openHAB website. See https://www.openhab.org/docs/developer/bindings/#add-your-binding-s-logo-to-the-openhab-website
* Add new binding liquidcheck Signed-off-by: Marcel Goerentz <[email protected]> Signed-off-by: Thomas Burri <[email protected]>
* Add new binding liquidcheck Signed-off-by: Marcel Goerentz <[email protected]> Signed-off-by: Matt Myers <[email protected]>
* Add new binding liquidcheck Signed-off-by: Marcel Goerentz <[email protected]> Signed-off-by: Jørgen Austvik <[email protected]>
Signed-off-by: Marcel Goerentz [email protected]
[liquidcheck] Initial contribution
Description
Adding a new binding so users can connect openHAB to Liquid-Check devices.
This binding integrates the sensor to the users home automation with little configuration effort.
Testing
Binding has been tested with hardware version B and firmware version 1.60 of the device.