-
-
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
[Homewizard] Initial contribution #9831
Conversation
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.
Thanks you for your contribution. I've added some review comments. There are 2 general comments. The way you handle configuration changes is overly complex. I've added several comments to make this much simpler. Secondly. You've added unit test case, which is great. But it doesn't add a lot. You're basically testing if gson works. Because you just read the output of a gson parsing. But that's not something that would need to be tested. A more useful, but also more complex test would be if given a data string would result in correctly updating a channel with a specific value.
...ding.homewizard/src/main/java/org/openhab/binding/homewizard/internal/HomeWizardHandler.java
Outdated
Show resolved
Hide resolved
...omewizard/src/main/java/org/openhab/binding/homewizard/internal/HomeWizardConfiguration.java
Outdated
Show resolved
Hide resolved
...ding.homewizard/src/main/java/org/openhab/binding/homewizard/internal/HomeWizardHandler.java
Outdated
Show resolved
Hide resolved
...ding.homewizard/src/main/java/org/openhab/binding/homewizard/internal/HomeWizardHandler.java
Outdated
Show resolved
Hide resolved
...ding.homewizard/src/main/java/org/openhab/binding/homewizard/internal/HomeWizardHandler.java
Outdated
Show resolved
Hide resolved
...ding.homewizard/src/main/java/org/openhab/binding/homewizard/internal/HomeWizardHandler.java
Outdated
Show resolved
Hide resolved
...ding.homewizard/src/main/java/org/openhab/binding/homewizard/internal/HomeWizardHandler.java
Outdated
Show resolved
Hide resolved
Processed the review comments. Removed the unit test for now. Signed-off-by: Daniël van Os <[email protected]>
Thank you for your comments and the time spent on the review. I've processed the comments and pushed the update. |
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 only have a few additional things that I'd like to mention. As @Hilbrand mentioned, you don't need to perform locking on your polling job since you are only using it in initialize
and dispose
, both of which will be called from the same thread.
bundles/org.openhab.binding.homewizard/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
...ding.homewizard/src/main/java/org/openhab/binding/homewizard/internal/HomeWizardHandler.java
Outdated
Show resolved
Hide resolved
...ding.homewizard/src/main/java/org/openhab/binding/homewizard/internal/HomeWizardHandler.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.
LGTM
....openhab.binding.homewizard/src/main/java/org/openhab/binding/homewizard/data/P1Payload.java
Outdated
Show resolved
Hide resolved
...ding.homewizard/src/main/java/org/openhab/binding/homewizard/internal/HomeWizardHandler.java
Outdated
Show resolved
Hide resolved
...ding.homewizard/src/main/java/org/openhab/binding/homewizard/internal/HomeWizardHandler.java
Outdated
Show resolved
Hide resolved
...ding.homewizard/src/main/java/org/openhab/binding/homewizard/internal/HomeWizardHandler.java
Outdated
Show resolved
Hide resolved
...ding.homewizard/src/main/java/org/openhab/binding/homewizard/internal/HomeWizardHandler.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.homewizard/src/main/resources/OH-INF/binding/binding.xml
Outdated
Show resolved
Hide resolved
There are some formatting issues. You can fix them with |
Hmm, that changed 10.560 files... I think this is related to the issue I reported: #9813 Probably the only file that should actually should change is the pom.xml file with the bindings in it. spotless:check reports only the one pom.xml file, spotless:apply messes up the line endings in 10560 files. |
731dd00
to
f0e0fd3
Compare
I'm at a loss here... When I run: mvn spotless:check -pl :org.openhab.binding.homewizard it comes up clean. When I run: mvn spotless:check It complains about files in a different binding (org.openhab.binding.tado) When I run: mvn spotless:apply It will modify thousands of files, which Git then refuses to commit because apart from line endings they are the same. |
I'm getting somewhere. I think there are 2 errors in one of the smallest file in the openHab sources... the .gitattributes file. The .java and .xml lines are missing the wildcards: *.xml / *.java As a result my commits retained their crlf's and my manual attempts only made it worse (git ls-files --eol in my addon dir illustrated this). Committed the updated .gitattributes file, ran git add --renormalize and things looked ok (lf in the index, crlf in workdir), just committed this. If CI now passes I have some cleaning up to do... |
There! No more warnings. I've cleaned up all those failed commits and signed off the squashed commit. Hopefully it still works. |
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
#9825 Initial contribution for a binding that connects to a HomeWizard P1 Wi-Fi meter.
The binding uses a local connection to query the device and adds one thing that exposes the obtained measurements.
The JAR is available here: https://drive.google.com/file/d/1DBn2yd0wZAnaz9pQmbDwk70XyQxNyQc_/view?usp=sharing
Signed-off-by: Daniël van Os [email protected]