-
-
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
[evohome] Add UoM and semantic tags for temperature channels #13885
Conversation
Signed-off-by: Leo Siepel <[email protected]>
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/evohome-binding-2-0/37264/276 |
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 for the refactoring. I have added some comments and questions (as I didn't pull branch and loaded into my IDE).
...binding.evohome/src/main/java/org/openhab/binding/evohome/internal/api/EvohomeApiClient.java
Show resolved
Hide resolved
...binding.evohome/src/main/java/org/openhab/binding/evohome/internal/api/EvohomeApiClient.java
Show resolved
Hide resolved
...binding.evohome/src/main/java/org/openhab/binding/evohome/internal/api/EvohomeApiClient.java
Show resolved
Hide resolved
...penhab.binding.evohome/src/main/java/org/openhab/binding/evohome/internal/api/ApiAccess.java
Outdated
Show resolved
Hide resolved
...penhab.binding.evohome/src/main/java/org/openhab/binding/evohome/internal/api/ApiAccess.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/openhab/binding/evohome/internal/handler/EvohomeAccountBridgeHandler.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/openhab/binding/evohome/internal/handler/EvohomeAccountBridgeHandler.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/openhab/binding/evohome/internal/handler/EvohomeAccountBridgeHandler.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/openhab/binding/evohome/internal/handler/EvohomeAccountBridgeHandler.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/openhab/binding/evohome/internal/handler/EvohomeAccountBridgeHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: lsiepel <[email protected]>
Signed-off-by: lsiepel <[email protected]>
Signed-off-by: lsiepel <[email protected]>
...penhab.binding.evohome/src/main/java/org/openhab/binding/evohome/internal/api/ApiAccess.java
Show resolved
Hide resolved
.../src/main/java/org/openhab/binding/evohome/internal/handler/EvohomeAccountBridgeHandler.java
Show resolved
Hide resolved
.../src/main/java/org/openhab/binding/evohome/internal/handler/EvohomeAccountBridgeHandler.java
Show resolved
Hide resolved
Signed-off-by: lsiepel <[email protected]>
Signed-off-by: lsiepel <[email protected]>
I think it would be good to have some actual tests as there are no tests to this bundle and null annotation refactoring have been the cause for many bugs :-) I'll also ask on the community forums, but without some enhancements not much users are willing to spend their time on it. |
.../src/main/java/org/openhab/binding/evohome/internal/handler/EvohomeAccountBridgeHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: lsiepel <[email protected]>
Signed-off-by: lsiepel <[email protected]>
Signed-off-by: lsiepel <[email protected]>
@jlaur Ready for merge as tests have been performed, all seems fine. |
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, only one question left.
.../src/main/java/org/openhab/binding/evohome/internal/handler/EvohomeAccountBridgeHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: lsiepel <[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.
Thanks!
@lsiepel - you could change the PR title to be about the UoM channel additions, and we could add "enhancement" label to have this mentioned in the release notes. |
Updated it, feel free to adjust it to release note needs. |
Signed-off-by: Leo Siepel <[email protected]>
Signed-off-by: Leo Siepel <[email protected]> Signed-off-by: miloit <[email protected]>
Signed-off-by: Leo Siepel <[email protected]>
Signed-off-by: Leo Siepel <[email protected]>
Signed-off-by: Leo Siepel [email protected]
Left the warnings about unused private fields untouched as these are part of the DTO responses and might be used in the future.
The jar (3.4.1) is here: https://1drv.ms/u/s!AnMcxmvEeupwjp4MT2ZumTKtulvBWg?e=yrmAmM
(updated after last commit as of 2023-01-07 15:19)
I keep this here as draft while some tests are being performed. As i don't own such a device, we have to wait for others to complete the testing.