-
-
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
[teleinfo] Initial contribution #7744
Conversation
Travis tests were successfulHey @olivierkeke, |
Travis tests were successfulHey @olivierkeke, |
2 similar comments
Travis tests were successfulHey @olivierkeke, |
Travis tests were successfulHey @olivierkeke, |
Travis tests have failedHey @olivierkeke, |
Travis tests were successfulHey @olivierkeke, |
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 saw that you put a lot of effort into this binding and you have my full respect to handle this complex protocol. I also like that you attached an UML diagram for documentation.
Here are some general points I found in the review:
There are several methods which log on entry and on exit. You could use the debugger for that purpose. See point 4 https://www.openhab.org/docs/developer/guidelines.html#f-logging
There are several checkstyle and compiler warnings left. You could take a look at target/code-analysis/report.html
.
There are some formatting issues. You can view them with mvn spotless:check -Dspotless.check.skip=false
and fix them with mvn spotless:apply
.
Please use null annotations. See https://www.openhab.org/docs/developer/guidelines.html#null-annotations The data classes can be moved into a package called dto
to prevent the missing null annotations.
bundles/org.openhab.binding.teleinfo/src/main/feature/feature.xml
Outdated
Show resolved
Hide resolved
...g.teleinfo/src/main/java/org/openhab/binding/teleinfo/internal/TeleinfoBindingConstants.java
Outdated
Show resolved
Hide resolved
...g.teleinfo/src/main/java/org/openhab/binding/teleinfo/internal/TeleinfoDiscoveryService.java
Outdated
Show resolved
Hide resolved
...g.teleinfo/src/main/java/org/openhab/binding/teleinfo/internal/TeleinfoDiscoveryService.java
Outdated
Show resolved
Hide resolved
...g.teleinfo/src/main/java/org/openhab/binding/teleinfo/internal/TeleinfoDiscoveryService.java
Outdated
Show resolved
Hide resolved
...org.openhab.binding.teleinfo/src/main/resources/ESH-INF/thing/common-cbemm-channel-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.teleinfo/src/main/resources/ESH-INF/thing/common-channel-types.xml
Show resolved
Hide resolved
bundles/org.openhab.binding.teleinfo/src/main/resources/ESH-INF/thing/common-channel-types.xml
Outdated
Show resolved
Hide resolved
...g.teleinfo/src/main/java/org/openhab/binding/teleinfo/internal/TeleinfoDiscoveryService.java
Outdated
Show resolved
Hide resolved
Travis tests were successfulHey @olivierkeke, |
5 similar comments
Travis tests were successfulHey @olivierkeke, |
Travis tests were successfulHey @olivierkeke, |
Travis tests were successfulHey @olivierkeke, |
Travis tests were successfulHey @olivierkeke, |
Travis tests were successfulHey @olivierkeke, |
Travis tests have failedHey @olivierkeke, |
Travis tests were successfulHey @olivierkeke, |
5 similar comments
Travis tests were successfulHey @olivierkeke, |
Travis tests were successfulHey @olivierkeke, |
Travis tests were successfulHey @olivierkeke, |
Travis tests were successfulHey @olivierkeke, |
Travis tests were successfulHey @olivierkeke, |
Travis tests have failedHey @olivierkeke, |
1 similar comment
Travis tests have failedHey @olivierkeke, |
Co-authored-by: Hilbrand Bouwkamp <[email protected]> Signed-off-by: Olivier Marceau <[email protected]>
Travis tests were successfulHey @olivierkeke, |
Signed-off-by: Olivier Marceau <[email protected]>
Travis tests were successfulHey @olivierkeke, |
1 similar comment
Travis tests were successfulHey @olivierkeke, |
Signed-off-by: Olivier Marceau <[email protected]>
Travis tests were successfulHey @olivierkeke, |
Signed-off-by: Olivier Marceau <[email protected]>
Travis tests were successfulHey @olivierkeke, |
Signed-off-by: Olivier Marceau <[email protected]>
Signed-off-by: Olivier Marceau <[email protected]>
Signed-off-by: Olivier Marceau <[email protected]>
Signed-off-by: Olivier Marceau <[email protected]>
Signed-off-by: Olivier Marceau <[email protected]>
Signed-off-by: Olivier Marceau <[email protected]>
Signed-off-by: Olivier Marceau <[email protected]>
@Hilbrand Thank you for the review. I think I've made the requested changes. |
Travis tests were successfulHey @olivierkeke, |
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.
Just one issue left related to the change of using ThingHandlerService
.
...src/main/java/org/openhab/binding/teleinfo/internal/handler/TeleinfoThingHandlerFactory.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/openhab/binding/teleinfo/internal/handler/TeleinfoThingHandlerFactory.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/openhab/binding/teleinfo/internal/handler/TeleinfoThingHandlerFactory.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Olivier Marceau <[email protected]>
Signed-off-by: Olivier Marceau <[email protected]>
Travis tests were successfulHey @olivierkeke, |
@Hilbrand Done! |
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
Great ! Many thanks for all ! |
Also-by: Nicolas SIBERIL <[email protected]> Signed-off-by: Olivier Marceau <[email protected]>
Also-by: Nicolas SIBERIL <[email protected]> Signed-off-by: Olivier Marceau <[email protected]>
Also-by: Nicolas SIBERIL <[email protected]> Signed-off-by: Olivier Marceau <[email protected]>
Related to #680 issue (titled "Add Teleinfo Binding") and Community thread: https://community.openhab.org/t/new-teleinfo-binding-tester-and-feedback-welcome/77857
Clean rebase of #7516