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.

Sign up for GitHub

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

Jump to bottom

[studer] Initial contribution #8163

Merged
merged 11 commits into from
Sep 7, 2020
Merged

[studer] Initial contribution #8163

merged 11 commits into from
Sep 7, 2020

Conversation

giovannimirulla
Copy link

Hi,
this is the initial contribution for Studer binding. Four things for four type of devices:

  • BSP
  • Xtender
  • VarioTrack
  • VarioString

Studer Innotec is an ISO certified company that develops and manufactures inverters, inverter/chargers and MPPT solar charge controllers. Goal of the binding is to support Studer devices.

Giovanni Mirulla

@giovannimirulla giovannimirulla requested a review from a team as a code owner July 20, 2020 23:26
@giovannimirulla giovannimirulla force-pushed the 2.5.x branch 2 times, most recently from 983db27 to 0fec996 Compare July 20, 2020 23:46
@TravisBuddy
Copy link

Travis tests were successful

Hey @giovannimirulla,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@bobadair bobadair added the new binding If someone has started to work on a binding. For a new binding PR. label Jul 21, 2020
fwolter
fwolter requested changes Jul 23, 2020
View reviewed changes
Copy link
Member

@fwolter fwolter 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! Great that you used Units of Measure so consequently! I reviewed your code and here is my feedback.

The pictures under doc/are acutally JPEGs, but the file extension is PNG.

There are some checkstyle 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.

@TravisBuddy
Copy link

Travis tests have failed

Hey @giovannimirulla,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@TravisBuddy
Copy link

Travis tests have failed

Hey @giovannimirulla,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

1 similar comment
@TravisBuddy
Copy link

Travis tests have failed

Hey @giovannimirulla,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@giovannimirulla giovannimirulla requested a review from fwolter July 25, 2020 01:58
fwolter
fwolter requested changes Jul 26, 2020
View reviewed changes
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

Your changes look good! I found a few minor things I overlooked during the first review. Sorry for that.

@TravisBuddy
Copy link

Travis tests were successful

Hey @giovannimirulla,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@giovannimirulla giovannimirulla requested a review from fwolter July 27, 2020 13:37
fwolter
fwolter approved these changes Jul 27, 2020
View reviewed changes
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

LGTM

@fwolter fwolter added the cre Coordinated Review Effort label Jul 27, 2020
@ssalonen ssalonen mentioned this pull request Jul 27, 2020
Merged
@giovannimirulla
Copy link
Author

@fwolter thank you for your help and your time!

Hilbrand
Hilbrand requested changes Aug 26, 2020
View reviewed changes
Copy link
Member

@Hilbrand Hilbrand 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 you contribution. Here is my review. Some comments also apply on other places, but I didn't comment each occurrence.

@Hilbrand
Copy link
Member

Hilbrand commented Sep 5, 2020

@giovannimirulla if you are able to fix my review comments on short notice wwe will be able to merge this in 2.5.9, otherwise it will be for openHAB 3.0

@giovannimirulla
[studer] Initial contribution
bc1c2b7 
Signed-off-by: Giovanni Mirulla <[email protected]>
@giovannimirulla
Revert "[studer] Initial contribution"
a072bc4 
This reverts commit 43bfe04.

Signed-off-by: Giovanni Mirulla <[email protected]>
@giovannimirulla
[studer] Initial contribution
11d65b2 
Signed-off-by: Giovanni Mirulla <[email protected]>
@giovannimirulla
Applied feedback from fwolter
af386ad 
Signed-off-by: Giovanni Mirulla <[email protected]>
@giovannimirulla
Preparation for next development cycle
272be34 
Signed-off-by: Giovanni Mirulla <[email protected]>
@giovannimirulla
Applied feedback from fwolter (few minor things)
c27bd64 
Signed-off-by: Giovanni Mirulla <[email protected]>
@giovannimirulla
Applied feedback from Hilbrand
db001b5 
Signed-off-by: Giovanni Mirulla <[email protected]>
@giovannimirulla
Update pom.xml
bd8046e 
Signed-off-by: Giovanni Mirulla <[email protected]>
Hilbrand
Hilbrand requested changes Sep 6, 2020
View reviewed changes
Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

Just 1 comment related to the rename of refresh.

You also need to rebase due to conflicts, see here on GitHub.

To make sure the style is correct run mvn spotless:apply on your binding code.

Hilbrand
Hilbrand approved these changes Sep 6, 2020
View reviewed changes
Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

LGTM

@giovannimirulla
Copy link
Author

@Hilbrand thank you for your help and your time!

@TravisBuddy
Copy link

Travis tests were successful

Hey @giovannimirulla,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

1 similar comment
@TravisBuddy
Copy link

Travis tests were successful

Hey @giovannimirulla,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@Hilbrand Hilbrand merged commit 7e30fa7 into openhab:2.5.x Sep 7, 2020
@Hilbrand Hilbrand added this to the 2.5.9 milestone Sep 7, 2020
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Sep 12, 2020
@giovannimirulla @CSchlipp
[studer] Initial contribution (openhab#8163)
00679b5 
Signed-off-by: Giovanni Mirulla <[email protected]>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
@giovannimirulla @markus7017
[studer] Initial contribution (openhab#8163)
005ecfa 
Signed-off-by: Giovanni Mirulla <[email protected]>
@ssalonen ssalonen mentioned this pull request Sep 22, 2020
Closed
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Oct 8, 2020
@giovannimirulla @andrewfg
[studer] Initial contribution (openhab#8163)
400687a 
Signed-off-by: Giovanni Mirulla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hilbrand Hilbrand Hilbrand approved these changes

@fwolter fwolter fwolter approved these changes

@ssalonen ssalonen Awaiting requested review from ssalonen

Assignees
No one assigned
Labels
cre Coordinated Review Effort new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Milestone
2.5.9
Development

Successfully merging this pull request may close these issues.
5 participants
@giovannimirulla @TravisBuddy @Hilbrand @fwolter @bobadair