[sunsynk] Initial contribution

[LeeC77]

Title

New binding for Sun Synk Connect
Initial contribution
This is the first time I have contributed and am a novice at using git and developing bindings, your understanding and patients would be appreciated.

Description

Adding binding.sunsynk to bunbles directory.
Separate pull request to cover POMS and Code Owners Here
Adds a new binding that communicated with the Sun Synk Web services in order to automate Electrical Power Inverter control

Testing

Built against 4.2.0-SNAPSHOT - Build #3989
Built JAR can be found [here] (https://github.com/LeeC77/sunsynk)

See Community [thread] (https://community.openhab.org/t/new-sun-synk-connect-account-and-inverter-binding/155680)

[lsiepel]

Please combine #16752 into this PR

[LeeC77]

Now combined (#16752) into this PR

[lsiepel]

Thanks for your contribution. I have seen some nice parts while performing a quick scan of this PR. It also has some places to improve, i left comments on those area's. Consider this as a first basic check of the PR.

For a quick review proces without many review rounds, it would help to perform some kind of self review with this checklist in mind

Edit: And a succesfull DCO is needed, better to fix that first.

[lsiepel]

Remove this file from the PR

[LeeC77]

This was a struggle (git quite new to me) hope it worked.

[lsiepel]

File is still here.

[LeeC77]

Hi All changes made , thanks for the review

[LeeC77]

Can someone give a bit more guidance to solve the DCO, new to Git, I thought I had signed.

Thanks for the help.

[lsiepel]

Can someone give a bit more guidance to solve the DCO, new to Git, I thought I had signed.

Thanks for the help.

If you click details next to the DCO you get to this page and that explains hopw to fix this. It seems like none of the commits are signed off. More generic info here:
https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits You can probably find some documentation sources for your specific IDE.

[lsiepel]

Left some comments that can be applied to multiple classes and situations, please try to check if each comment can be applied eelsewhere.
Similar comments for the classes packages, remove commented code, change folder/package name.

• while revieweing, i see some commits are added that also add code from other bindings. Hope you manage to fix that too. Ping me if you need any help

[LeeC77]

Thanks @lsiepel, have now signed off all commits as request, DCO looks to have completed. Followed guidance given.

In your local branch, run: git rebase HEAD~31 --signoff
Force push your changes to overwrite the branch: git push --force-with-lease origin sunsynk

Hope that has not caused problems for other contributors

[morph166955]

Please rebase. You've got some other merged PRs in here that need to be resolved.

[LeeC77]

@lsiepel, Just the one open comment left. If you really need it I can do it.

Regards [ i would propose to create an ApiController class that is responsible for all calls to the remove service, these can be extracted from the ThiongHandlers ]
Looks to be a lot of work for little gain and the design pattern I have used is similar to many of the other bindings.

Can I thank you for your support so far on this my very first contribution; while I am a Java and Git novice you have been patient and have encouraged me to grow. I have learnt a lot.

[lsiepel]

Please rebase. You've got some other merged PRs in here that need to be resolved.

Seems still an issue, le tme know if you need any guidance on fixing this. Please rebase.

@lsiepel, Just the one open comment left. If you really need it I can do it.

Regards [ i would propose to create an ApiController class that is responsible for all calls to the remove service, these can be extracted from the ThiongHandlers ] Looks to be a lot of work for little gain and the design pattern I have used is similar to many of the other bindings.

Can I thank you for your support so far on this my very first contribution; while I am a Java and Git novice you have been patient and have encouraged me to grow. I have learnt a lot.

We have all been there, happy to help. Regarding the ApiController, i do recommend to change this now. If needed i can help or guide you, but seperation of concerns is one of the most important 'pattern rules' for a project like this to have maintainable and testable code. Again, if you need any help, just ping me, i'm sure others are also willing to help.

[LeeC77]

@lsiepel
I have tried to rebase, how do I tell if it has worked?
if its not worked, I may need to some guidance.
I did this in my local branch:

git checkout main
git pull
checkout sunsynk
rebase main
checkout main
rebase sunsynk

Think I'm going to need some guidance on the proposed ApiController. Could we start with your suggestion how to divide function with modules and file location so I can address what you mean by separation of concerns.

Also I'm going to have to look up custom exceptions, I think that was one of the other things you requested.

[lsiepel]

@lsiepel I have tried to rebase, how do I tell if it has worked?
need some guidance on the proposed ApiController. Could we start with your suggestion how to divide function with modules and file location so I can address what you mean by separation of concerns.

Also I'm going to have to look up custom exceptions, I think that was one of the other things you requested.

Yep, that worked. Multiple methods to check, but you can always see the result in this PR: https://github.com/openhab/openhab-addons/pull/16753/files (if it doesnt contain files from other commits your good.

The ApiController would be responsible for all communication to the remote service. It would contain one or more public methods that can be used by the handler to get the data (e,g getData(). This method typically has several steps;

• check if authenticated
• authenticate
• refresh token
• create request
• execute request
• return data (a dto or list of dto's)

Each step can be inline or modelled as private method. These method should throw custom exceptions (e.g ApiAuthenticationExcepetion for problems when authenticating).

Many bindings use this pattern, maybe inspect PlugwiseHA that uses both apicontroller and custom exceptions.

Did you see the build fails due to spotless issues?