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

[ojelectronics] Initial contribution #7138

Merged
merged 11 commits into from
Jul 11, 2020
Merged

[ojelectronics] Initial contribution #7138

merged 11 commits into from
Jul 11, 2020

Conversation

EvilPingu
Copy link
Contributor

@EvilPingu EvilPingu commented Mar 9, 2020

Hi,

this is the initial contribution for ojelectronics binding. First of all i build two things:

  • OJCloud: Handles the traffic with the ojelectronics cloud
  • Thermostat: Have the (read only) channel for the associated thermostats

OJ Electronics builds floor heating thermostats. Goal of the binding is to support OJ Electronics devices.

EvilPingu

@TravisBuddy
Copy link

Travis tests were successful

Hey @EvilPingu,
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 @EvilPingu,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@EvilPingu EvilPingu changed the title [ojelectronics][WIP] Initial contribution [ojelectronics] Initial contribution Mar 10, 2020
@cpmeister cpmeister added the new binding If someone has started to work on a binding. For a new binding PR. label Mar 10, 2020
@TravisBuddy
Copy link

Travis tests were successful

Hey @EvilPingu,
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 @EvilPingu,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@TravisBuddy
Copy link

Travis tests have failed

Hey @EvilPingu,
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.

1st Build

Expand here
Single addon pull request: Building org.openhab.binding.ojelectronics
[ERROR] Failed to execute goal org.commonjava.maven.plugins:directory-maven-plugin:0.3.1:directory-of (directories) on project org.openhab.binding.ojelectronics: Cannot find directory for project: org.openhab.addons:org.openhab.addons.reactor -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException

### 2nd Build
Expand here
Single addon pull request: Building org.openhab.binding.ojelectronics
[WARNING] The POM for com.sun.xml.bind:jaxb-core:jar:2.2.11 is invalid, transitive dependencies (if any) will not be available, enable debug logging for more details
[WARNING] The POM for com.sun.xml.bind:jaxb-impl:jar:2.2.11 is invalid, transitive dependencies (if any) will not be available, enable debug logging for more details
[ERROR] Failed to execute goal org.commonjava.maven.plugins:directory-maven-plugin:0.3.1:directory-of (directories) on project org.openhab.binding.ojelectronics: Cannot find directory for project: org.openhab.addons:org.openhab.addons.reactor -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException

@fwolter
Copy link
Member

fwolter commented Jun 6, 2020

Can you fix the conflicting file and the build errors? I would take a look into it, then.

@EvilPingu EvilPingu requested a review from a team as a code owner June 20, 2020 09:02
@TravisBuddy
Copy link

Travis tests were successful

Hey @EvilPingu,
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 @EvilPingu,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@EvilPingu
Copy link
Contributor Author

EvilPingu commented Jun 20, 2020

Can you fix the conflicting file and the build errors? I would take a look into it, then.

Hi @fwolter ,

conflicts are solved now.

EvilPingu

@TravisBuddy
Copy link

Travis tests were successful

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

2 similar comments
@TravisBuddy
Copy link

Travis tests were successful

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

@TravisBuddy
Copy link

Travis tests were successful

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

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.

Very professional code. I like the usage of Jetty's async API and the lambdas.

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.

bundles/org.openhab.binding.ojelectronics/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.ojelectronics/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.ojelectronics/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.ojelectronics/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.ojelectronics/README.md Outdated Show resolved Hide resolved
bundles/pom.xml Outdated Show resolved Hide resolved
bundles/pom.xml Outdated Show resolved Hide resolved
@TravisBuddy
Copy link

Travis tests have failed

Hey @EvilPingu,
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 @EvilPingu,
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 were successful

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

@EvilPingu
Copy link
Contributor Author

Very professional code. I like the usage of Jetty's async API and the lambdas.

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.

Hi @fwolter,

what are the next steps? How do we get on? Did I miss something?

@TravisBuddy
Copy link

Travis tests were successful

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

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 the hint. Github doesn't notify when comments are set to resolved.

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

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

bundles/org.openhab.binding.ojelectronics/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.ojelectronics/README.md Outdated Show resolved Hide resolved
</config-description>
</thing-type>
<channel-type id="floorTemperature">
<item-type>Number</item-type>
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to adopt or comment on this?

bundles/pom.xml Outdated Show resolved Hide resolved
@TravisBuddy
Copy link

Travis tests were successful

Hey @EvilPingu,
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 @EvilPingu,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@EvilPingu
Copy link
Contributor Author

Thanks for the hint. Github doesn't notify when comments are set to resolved.

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

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

Hi @fwolter,

i fixed all review things and i also checked target/code-analysis/report.html. It's empty.

@EvilPingu

@TravisBuddy
Copy link

Travis tests were successful

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

@fwolter
Copy link
Member

fwolter commented Jul 1, 2020

Did you run mvn install? I see 15 warnings:
grafik

@EvilPingu
Copy link
Contributor Author

Did you run mvn install? I see 15 warnings:
grafik

I didn't understand this. I'm confused. I make "mvn clean install". No errors, no warning: file:///home/ckit/dev/openhab-2-5-x/git/openhab-addons/target/code-analysis/report.html and file:///home/ckit/dev/openhab-2-5-x/git/openhab-addons/target/summary_report.html

@fwolter
Copy link
Member

fwolter commented Jul 1, 2020

Did you push your changes? For example RefreshService:56, the field name is still lower case.

Signed-off-by: EvilPingu <ckittel@gmx.de>
@TravisBuddy

This comment has been minimized.

Signed-off-by: EvilPingu <ckittel@gmx.de>
@TravisBuddy
Copy link

Travis tests were successful

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

Signed-off-by: EvilPingu <ckittel@gmx.de>
Signed-off-by: EvilPingu <ckittel@gmx.de>
Signed-off-by: EvilPingu <ckittel@gmx.de>
Signed-off-by: EvilPingu <ckittel@gmx.de>
Signed-off-by: EvilPingu <ckittel@gmx.de>
@TravisBuddy
Copy link

Travis tests were successful

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

Signed-off-by: EvilPingu <ckittel@gmx.de>
@TravisBuddy
Copy link

Travis tests were successful

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

@martinvw martinvw merged commit 5d2d6f0 into openhab:2.5.x Jul 11, 2020
@martinvw martinvw added this to the 2.5.7 milestone Jul 11, 2020
@martinvw martinvw removed the cre Coordinated Review Effort label Jul 11, 2020
@martinvw
Copy link
Member

Thanks a lot!

@EvilPingu
Copy link
Contributor Author

EvilPingu commented Jul 11, 2020

It was a great pleasure for me. I learned a lot. 👍 Thanks for the support.

knikhilwiz pushed a commit to knikhilwiz/openhab2-addons that referenced this pull request Jul 12, 2020
Signed-off-by: EvilPingu <ckittel@gmx.de>
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Jul 26, 2020
Signed-off-by: EvilPingu <ckittel@gmx.de>
Signed-off-by: CSchlipp <christian@schlipp.de>
MPH80 pushed a commit to MPH80/openhab-addons that referenced this pull request Aug 3, 2020
Signed-off-by: EvilPingu <ckittel@gmx.de>
Signed-off-by: MPH80 <michael@hazelden.me>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Signed-off-by: EvilPingu <ckittel@gmx.de>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Signed-off-by: EvilPingu <ckittel@gmx.de>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Signed-off-by: EvilPingu <ckittel@gmx.de>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Signed-off-by: EvilPingu <ckittel@gmx.de>
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
Signed-off-by: EvilPingu <ckittel@gmx.de>
Signed-off-by: Daan Meijer <daan@studioseptember.nl>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
Signed-off-by: EvilPingu <ckittel@gmx.de>
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.

7 participants