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

[oppo] rollforward a few review changes from sister bindings and fix a title #8335

Merged
merged 14 commits into from
Aug 26, 2020
Merged

[oppo] rollforward a few review changes from sister bindings and fix a title #8335

merged 14 commits into from
Aug 26, 2020

Conversation

mlobstein
Copy link
Contributor

No description provided.

@TravisBuddy
Copy link

Travis tests were successful

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

lolodomo
lolodomo requested changes Aug 24, 2020
View reviewed changes
...ing.oppo/src/main/java/org/openhab/binding/oppo/internal/communication/OppoReaderThread.java Outdated
@@ -42,7 +42,7 @@
* @param connector the object that should handle the received message
*/
public OppoReaderThread(OppoConnector connector, String uid) {
super(OppoBindingConstants.BINDING_ID + "-" + uid);
super("OH-binding-" + OppoBindingConstants.BINDING_ID + "-" + uid);
Copy link
Contributor

Choose a reason for hiding this comment

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

You do not need the binding id as already included in uid (thing uid).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variable was named poorly so I corrected it. The idea was to help identify the particular connection (IP+ port or serial port name) in the case of multiple instances of the binding being used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Look at #8216, you are not following the new thread naming convention. You have to replace binding id by thing uid. Then having the serial port name as custom part in the name is ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, it should be fixed now.

@mlobstein
review changes
6f287e2 
Signed-off-by: Michael Lobstein <[email protected]>
@TravisBuddy
Copy link

Travis tests were successful

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

@mlobstein mlobstein requested a review from lolodomo August 24, 2020 21:58
@mlobstein
review changes
fb7f65b 
Signed-off-by: Michael Lobstein <[email protected]>
@TravisBuddy
Copy link

Travis tests were successful

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

@mlobstein
review changes
93224d6 
Signed-off-by: Michael Lobstein <[email protected]>
@TravisBuddy
Copy link

Travis tests were successful

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

@mlobstein
fix thread naming convention
465d1e6 
Signed-off-by: Michael Lobstein <[email protected]>
@TravisBuddy
Copy link

Travis tests were successful

Hey @mlobstein,
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 approved these 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.

LGTM

@Hilbrand Hilbrand merged commit ac3ac2b into openhab:2.5.x Aug 26, 2020
@Hilbrand Hilbrand added this to the 2.5.9 milestone Aug 26, 2020
@mlobstein mlobstein deleted the OppoFix branch August 26, 2020 14:29
@lolodomo
Copy link
Contributor

You used the thing type uid rather than the thing uid,

@mlobstein mlobstein mentioned this pull request Aug 26, 2020
Merged
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@mlobstein @andrewfg
[oppo] rollforward a few review changes from sister bindings and fix …
8a4aeee 
…a title (openhab#8335)

* Rollforward a few review changes
* fix thread naming convention

Signed-off-by: Michael Lobstein <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@mlobstein @andrewfg
[oppo] rollforward a few review changes from sister bindings and fix …
378b98e 
…a title (openhab#8335)

* Rollforward a few review changes
* fix thread naming convention

Signed-off-by: Michael Lobstein <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@mlobstein @andrewfg
[oppo] rollforward a few review changes from sister bindings and fix …
06c1157 
…a title (openhab#8335)

* Rollforward a few review changes
* fix thread naming convention

Signed-off-by: Michael Lobstein <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@mlobstein @andrewfg
[oppo] rollforward a few review changes from sister bindings and fix …
ed582ae 
…a title (openhab#8335)

* Rollforward a few review changes
* fix thread naming convention

Signed-off-by: Michael Lobstein <[email protected]>
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
@mlobstein @DaanMeijer
[oppo] rollforward a few review changes from sister bindings and fix …
194180a 
…a title (openhab#8335)

* Rollforward a few review changes
* fix thread naming convention

Signed-off-by: Michael Lobstein <[email protected]>
Signed-off-by: Daan Meijer <[email protected]>
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Sep 12, 2020
@mlobstein @CSchlipp
[oppo] rollforward a few review changes from sister bindings and fix …
4d74b41 
…a title (openhab#8335)

* Rollforward a few review changes
* fix thread naming convention

Signed-off-by: Michael Lobstein <[email protected]>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
@mlobstein @markus7017
[oppo] rollforward a few review changes from sister bindings and fix …
779f06b 
…a title (openhab#8335)

* Rollforward a few review changes
* fix thread naming convention

Signed-off-by: Michael Lobstein <[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

@lolodomo lolodomo Awaiting requested review from lolodomo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.5.9
Development

Successfully merging this pull request may close these issues.
4 participants
@mlobstein @TravisBuddy @lolodomo @Hilbrand