Skip to content
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

[Easee] Initial contribution #12954

Merged
merged 72 commits into from
Sep 10, 2022
Merged

[Easee] Initial contribution #12954

merged 72 commits into from
Sep 10, 2022

Conversation

alexf2015
Copy link
Contributor

The Easee binding can be used to retrieve data from the Easee Cloud API and also to control your wallbox via the Cloud API. This allows you to dynamically adjust the charge current for your car depending on production of your solar plant.

Further details can be found in the corresponding README.

@alexf2015 alexf2015 requested a review from a team as a code owner June 17, 2022 15:57
@alexf2015 alexf2015 added the new binding If someone has started to work on a binding. For a new binding PR. label Jun 17, 2022
@lolodomo
Copy link
Contributor

Does not build.

@alexf2015
Copy link
Contributor Author

I do not understand the error. I builds successfully both on my local machine and on my Jenkins CI.

@alexf2015 alexf2015 force-pushed the easee-dev branch 6 times, most recently from 9a7ed27 to 19a290a Compare July 17, 2022 18:06
@jlaur jlaur changed the title [Easee] New Binding / Initial contribution [Easee] Initial contribution Jul 25, 2022
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution, the code already looks really good! I have reviewed it, and provided some feedback.

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.

The I18N properties file must be regenerated. For reference: https://www.openhab.org/docs/developer/utils/i18n.html#generating-i18n-properties-file

The DCO is not passing, this needs to be fixed.

@alexf2015 alexf2015 force-pushed the easee-dev branch 4 times, most recently from 777d9a1 to 7efa8a1 Compare August 20, 2022 07:34
Signed-off-by: Alexander Friese <af944580@gmail.com>
@alexf2015 alexf2015 force-pushed the easee-dev branch 3 times, most recently from 98181b4 to 0e0e4a4 Compare August 21, 2022 17:57
@alexf2015 alexf2015 requested a review from jlaur August 21, 2022 17:58
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have now been through all previous comments and should be up-to-date. I have reopened a few and also provided some additional feedback. I would like to go through all changed files, but didn't find time for all of it today, so please consider as partial for now.

There are still some checkstyle warnings left. You could take a look at target/code-analysis/report.html.

@alexf2015 alexf2015 force-pushed the easee-dev branch 2 times, most recently from c826c06 to 8f4c975 Compare September 2, 2022 17:28
Alexander Friese added 2 commits September 2, 2022 19:42
Signed-off-by: Alexander Friese <af944580@gmail.com>
Signed-off-by: Alexander Friese <af944580@googlemail.com>
Signed-off-by: Alexander Friese <af944580@gmail.com>
Signed-off-by: Alexander Friese <af944580@googlemail.com>
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added inline comment for the SAT findings.

@jlaur
Copy link
Contributor

jlaur commented Sep 6, 2022

@openhab/add-ons-maintainers - does anyone know if SLF4J_LOGGER_SHOULD_BE_NON_STATIC is intentional or false positive?

I randomly found this commit that at least documents that it was changed from static to non-static in 2017, but I don't think it's possible to find the related issue anymore in the old repo: openhab/openhab2-addons@4bb37ad

@wborn
Copy link
Member

wborn commented Sep 7, 2022

The use of non-static loggers is part of the coding guidelines:

https://www.openhab.org/docs/developer/guidelines.html#f-logging

@wborn
Copy link
Member

wborn commented Sep 7, 2022

You can find the OH2 issues in this repo: #2109

@wborn
Copy link
Member

wborn commented Sep 7, 2022

the code analysis report still links to this findbugs section which no longer exists

These findings are generated by a plugin so the section never existed in any findbugs/spotbugs documentation. We should update SAT to link to the correct documentation for these findings, e.g.:

https://www.kengo-toda.jp/findbugs-slf4j/#slf4j_logger_should_be_non_static

Signed-off-by: Alexander Friese <af944580@googlemail.com>
@alexf2015
Copy link
Contributor Author

The use of non-static loggers is part of the coding guidelines:

https://www.openhab.org/docs/developer/guidelines.html#f-logging

I changed my code but I am in doubt that this is better style...

Signed-off-by: Alexander Friese <af944580@googlemail.com>
Signed-off-by: Alexander Friese <af944580@googlemail.com>
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the latest README updates I went over the formatted version, so provided some formatting and proof-reading comments. These are my last comments for this PR, so we should be ready to merge soon.

bundles/org.openhab.binding.easee/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.easee/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.easee/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.easee/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.easee/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.easee/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.easee/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.easee/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.easee/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.easee/README.md Outdated Show resolved Hide resolved
Signed-off-by: Alexander Friese <af944580@googlemail.com>
Signed-off-by: Alexander Friese <af944580@googlemail.com>
@jlaur
Copy link
Contributor

jlaur commented Sep 9, 2022

@alexf2015 - there are two more open comments above. I see that they are the ones GitHub has chosen to hide, so wondering if you might have missed them for this reason? You might also simply still be considering them, just wanted to give you a heads up to not prolong the review.

@alexf2015
Copy link
Contributor Author

I was just about to change it. Meanwhile you commited the changes already. I need to signoff. Then it's done.

Signed-off-by: Alexander Friese <af944580@googlemail.com>
@jlaur
Copy link
Contributor

jlaur commented Sep 10, 2022

I was just about to change it. Meanwhile you commited the changes already. I need to signoff. Then it's done.

Yes, sorry for that, I committed at the same time of your comment reply. Don't worry about the DCO, I can fix that when merging. Do you want to separate into two columns or not?

@alexf2015
Copy link
Contributor Author

It is ok now. I had just separated it but I am totally fine with it as it is. If you use textual configuration it is more convenient as it is now.

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for your contribution.

Now, you could add your binding's logo to the openHAB website. See https://www.openhab.org/docs/developer/bindings/#add-your-binding-s-logo-to-the-openhab-website

@jlaur jlaur merged commit 1f1de41 into openhab:main Sep 10, 2022
@jlaur jlaur added this to the 3.4 milestone Sep 10, 2022
@lolodomo
Copy link
Contributor

lolodomo commented Oct 1, 2022

@alexf2015 : you forgot to update the CODEOWNERS file. Please create a PR to add a line in that file with you as code maintainer for this new binding.

leifbladt pushed a commit to leifbladt/openhab-addons that referenced this pull request Oct 15, 2022
* initial binding version

Signed-off-by: Alexander Friese <af944580@gmail.com>
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
* initial binding version

Signed-off-by: Alexander Friese <af944580@gmail.com>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
* initial binding version

Signed-off-by: Alexander Friese <af944580@gmail.com>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
marcelGoerentz pushed a commit to marcelGoerentz/openhab-addons that referenced this pull request Nov 14, 2022
* initial binding version

Signed-off-by: Alexander Friese <af944580@gmail.com>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
* initial binding version

Signed-off-by: Alexander Friese <af944580@gmail.com>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
* initial binding version

Signed-off-by: Alexander Friese <af944580@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants