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

[cloudrain] Initial contribution #11239

Closed
wants to merge 3 commits into from
Closed

Conversation

Tikoell
Copy link

@Tikoell Tikoell commented Sep 11, 2021

Initial contribution for a new Cloudrain binding.

Cloudrain offers a smart irrigation system which uses weather data to optimize irrigation schedules. The binding allows you to integrate, view and control Cloudrain devices in the openHAB environment with the help of the Cloudrain Developer API.

Link to the jar file
Link to the source

Signed-off-by: Till Koellmann [email protected]

@Tikoell Tikoell requested a review from a team as a code owner September 11, 2021 20:50
Initial contribution for a Cloudrain binding (https://cloudrain.de)
Signed-off-by: Till Koellmann <[email protected]>
Signed-off-by: tikoell <[email protected]>
@Skinah Skinah added the new binding If someone has started to work on a binding. For a new binding PR. label Sep 14, 2021
Copy link
Contributor

@Skinah Skinah left a comment

Choose a reason for hiding this comment

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

Thanks for the submission, once you have done these I will take another look. Whilst waiting, please also go into the \target\code-analysis folder and read the report and fix anything in there.

Copy link
Contributor

@soenkekueper soenkekueper left a comment

Choose a reason for hiding this comment

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

Hey,

thanks for your contribution!

Code looks good, there are just some typos and smaller remarks.

Due this was my first code review for openhab-addons and i'm not (yet) familiar with all the things around in the
openhab-environment, i've just finished the review as comment, so feel free to skip some remarks, if they don't make sense to you.

@Tikoell
Copy link
Author

Tikoell commented Oct 27, 2021

Thanks for the reviews! I will incorporate the changes by end of next week.

@Tikoell
Copy link
Author

Tikoell commented Nov 13, 2021

Hi, thanks for the reviews. I fixed all findings and commented on your questions. Please have a look again. Thanks!

@Tikoell Tikoell requested a review from Skinah November 14, 2021 17:54
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

Here comes my review feedback. @Skinah and @soenkekueper already did a good job!

Comment on lines +83 to +84
| duration | Integer | The total duration in seconds of an active irrigation. NULL if no irrigation is active. |
| remainingSeconds | Integer | The remaining duration in seconds of an active irrigation. NULL if no irrigation is active. |
Copy link
Member

Choose a reason for hiding this comment

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

You could use Units of Measure.

Also, Integer is not a valid Item type.

Suggested change
| duration | Integer | The total duration in seconds of an active irrigation. NULL if no irrigation is active. |
| remainingSeconds | Integer | The remaining duration in seconds of an active irrigation. NULL if no irrigation is active. |
| duration | Number:Time | The total duration in seconds of an active irrigation. NULL if no irrigation is active. |
| remainingSeconds | Number:Time | The remaining duration in seconds of an active irrigation. NULL if no irrigation is active. |

Comment on lines +157 to +161
// Register the zone to receive irrigation updates only if channels are linked
// This is done to avoid unnecessary frequent API calls
if (isAnyIrrigationChannelLinked()) {
handler.registerForIrrigationUpdates(zoneId, getThing());
}
Copy link
Member

Choose a reason for hiding this comment

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

You could do this more easily without retrieving the registry.

getThing().getChannels().stream().filter(channel -> isLinked(channel.getUID())).findAny().isPresent();

Comment on lines +182 to +186
@Override
public void handleRemoval() {
super.handleRemoval();
unregisterAll();
}
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for implementing this method?

xsi:schemaLocation="https://openhab.org/schemas/binding/v1.0.0 https://openhab.org/schemas/binding-1.0.0.xsd">

<name>Cloudrain Binding</name>
<description>This is the binding for the Cloudrain system.</description>
Copy link
Member

Choose a reason for hiding this comment

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

Could you be a bit more verbose, so that somebody who hasn't heard about it gets a clue what this is about?

Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be adapted to the new addon.xml, the PR to adapt the developer docs is not merged yet, so if you need more details, the corresponding issue is here: openhab/openhab-core#2058
But you can also update your branch and look at another binding's addon.xml and adapt it to this initial contribution.

@fwolter
Copy link
Member

fwolter commented Jun 19, 2022

@Tikoell What's the status of this PR?

<parent>
<groupId>org.openhab.addons.bundles</groupId>
<artifactId>org.openhab.addons.reactor.bundles</artifactId>
<version>3.2.0-SNAPSHOT</version>
Copy link
Member

@wborn wborn Jul 10, 2022

Choose a reason for hiding this comment

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

Suggested change
<version>3.2.0-SNAPSHOT</version>
<version>4.2.0-SNAPSHOT</version>

@jlaur jlaur added the awaiting feedback Awaiting feedback from the pull request author label Oct 18, 2022
@@ -0,0 +1,17 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://maven.apache.org/POM/4.0.0"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">

See #11833

@openhab openhab deleted a comment from fwolter Jul 24, 2023
@lsiepel
Copy link
Contributor

lsiepel commented Dec 24, 2023

Hi @Tikoell are you planning to proceed with this PR? it has been around here for quite some time now and i'm happy to review or help you out, but if you can't proceed, it might be better to just close this.

@lsiepel lsiepel added the stale As soon as a PR is marked stale, it can be removed 6 months later. label Apr 29, 2024
@lsiepel
Copy link
Contributor

lsiepel commented May 31, 2024

Unfortunately it looks like we cannot bring this PR to the finish line. After two years of inactivity i close this PR. If there is a need to restart, feel free to re-open or start a new PR.

@lsiepel lsiepel closed this May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback Awaiting feedback from the pull request author new binding If someone has started to work on a binding. For a new binding PR. stale As soon as a PR is marked stale, it can be removed 6 months later.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants