-
-
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
[somfymylink] Somfy MyLink Binding initial contribution #5652
Conversation
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/somfy-mylink-somfy-synergy-api-binding/60415/7 |
Any chance of a review pass on this so I can make any adjustments needed? |
I‘ll try to have a look today. |
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. I'll do a more detailed review later. But you can start with writing the readme. And adding your binding to the CODEOWNERS
, bom/openhab2-addons/pom.xml
and create a feature.xml
file. All these file changes are also done when creating a basis binding with the skeleton script. So you could use the script to make the changes and copy the changes to your binding.
Also make sure the coding style is correct in all java files. Eclipse should take care off it when saving a file, or use ctrl-shift-f in eclipse.
Thanks @Hilbrand will do! this is my first time ... so i appreciate the guidance! |
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.
Here is my review. The code looks good. Mostly some style issues, which some repeated on several places, which causes the high number of comments. With Ctrl-Shift-F (auto format) in Eclipse you should be able to solve a number of my comments. Also check the report html generated when you do a maven build of the binding.
About the scenelist on the bridge. Is that something that doesn't change after a thing has been added? Because it now only gets the list when the thing is created. But I would assume this is something dynamic and can change when the user changes the scenes? I also don't know what it's for on the bridge. The channel is readonly, this means the ui might show only one item. It's then not possible to open the list.
I'll will have a second look at the discovery class later, as the way it registers itself as service can be improved to follow the preferred conventions.
...mylink/src/main/java/org/openhab/binding/somfymylink/internal/SomfyMyLinkHandlerFactory.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/openhab/binding/somfymylink/internal/handler/SomfyMyLinkBridgeHandler.java
Show resolved
Hide resolved
...src/main/java/org/openhab/binding/somfymylink/internal/handler/SomfyMyLinkBridgeHandler.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/openhab/binding/somfymylink/internal/handler/SomfyMyLinkBridgeHandler.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/openhab/binding/somfymylink/internal/handler/SomfyMyLinkBridgeHandler.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/openhab/binding/somfymylink/internal/handler/SomfyMyLinkBridgeHandler.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.somfymylink/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
Hi Chris, @LoungeFlyZ any progress on the review comments or do you need any help? |
@Hilbrand apologies for the late reply (been travelling). I am a VS Code user and need to work out what the major formatting issues are so i can manually apply them vs. in Eclipse. I'm not a pro Java dev and struggle with Eclispe and had more luck with VS Code. Anyway, this is not your problem and i just need to figure out what the style rules are and get them sorted. I'll try and get time shortly to do that. Thanks for the offer of help. |
@LoungeFlyZ good news. The tool spotless has been integrated. You can use this to reformat the code with maven. Rebase you code base and you can apply the tool. See: https://www.openhab.org/docs/developer/guidelines.html#b-code-formatting-rules-style |
...src/main/java/org/openhab/binding/somfymylink/internal/handler/SomfyMyLinkBridgeHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Chris Johnson <chrisfjohnson@live.com>
Signed-off-by: Chris Johnson <chrisfjohnson@live.com>
… classes Signed-off-by: Chris Johnson <chrisfjohnson@live.com>
…riter to avoid buffers for writing to socket Signed-off-by: Chris Johnson <chrisfjohnson@live.com>
Signed-off-by: Chris Johnson <chrisfjohnson@live.com>
…try blocks Signed-off-by: Chris Johnson <chrisfjohnson@live.com>
Signed-off-by: Chris Johnson <chrisfjohnson@live.com>
Signed-off-by: Chris Johnson <chrisfjohnson@live.com>
Signed-off-by: Chris Johnson <chrisfjohnson@live.com>
@cpmeister updated. also attempted to fix the missing DCO's but doesnt look like it worked. I accepted a change suggested via the github UI and it seems to break the DCO? |
Manually checked sign-off. |
Thanks again to @cpmeister, @Hilbrand and @mhilbush for helping me get this contribution ready. I very much appreciated all your support and assistance. Thanks again. |
* Initial contribution Signed-off-by: Chris Johnson <chrisfjohnson@live.com> Co-authored-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
* Initial contribution Signed-off-by: Chris Johnson <chrisfjohnson@live.com> Co-authored-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
@LoungeFlyZ the somfymylink is still missing a |
* Initial contribution Signed-off-by: Chris Johnson <chrisfjohnson@live.com> Co-authored-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
* Initial contribution Signed-off-by: Chris Johnson <chrisfjohnson@live.com> Co-authored-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com> Signed-off-by: CSchlipp <christian@schlipp.de>
* Initial contribution Signed-off-by: Chris Johnson <chrisfjohnson@live.com> Co-authored-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
* Initial contribution Signed-off-by: Chris Johnson <chrisfjohnson@live.com> Co-authored-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
* Initial contribution Signed-off-by: Chris Johnson <chrisfjohnson@live.com> Co-authored-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
* Initial contribution Signed-off-by: Chris Johnson <chrisfjohnson@live.com> Co-authored-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
* Initial contribution Signed-off-by: Chris Johnson <chrisfjohnson@live.com> Co-authored-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com> Signed-off-by: Daan Meijer <daan@studioseptember.nl>
* Initial contribution Signed-off-by: Chris Johnson <chrisfjohnson@live.com> Co-authored-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
This is a binding to support the Somfy MyLink device for control of RTS blinds/shades.
It supports control of individual motors as well as scenes.
This PR Fixes #4438
Many thanks to @mhilbush for his wonderful guidance on development of my first binding.
Signed-off-by: Chris Johnson chrisfjohnson@live.com