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

[tellstick] Add null annotations #17974

Merged
merged 11 commits into from
Feb 23, 2025
Merged

[tellstick] Add null annotations #17974

merged 11 commits into from
Feb 23, 2025
+192 −115

Conversation

lsiepel
Copy link
Contributor

@lsiepel lsiepel commented Dec 24, 2024

  • Add null annotations
  • Fixed most warnings as a result of adding null annotations.

@lsiepel lsiepel requested a review from a team as a code owner December 24, 2024 20:09
jlaur
jlaur requested changes Dec 28, 2024
View reviewed changes
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 fixing the annotations and warnings. Can you rebase to make the dependency upgrades disappear? 🙂

lsiepel and others added 5 commits December 28, 2024 14:57
@lsiepel @jlaur
Update bundles/org.openhab.binding.tellstick/src/main/java/org/openha…
f62861b 
…b/binding/tellstick/internal/core/TelldusCoreBridgeHandler.java

Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: lsiepel <[email protected]>
@lsiepel @jlaur
Update bundles/org.openhab.binding.tellstick/src/main/java/org/openha…
1329b3e 
…b/binding/tellstick/internal/core/TelldusCoreBridgeHandler.java

Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: lsiepel <[email protected]>
@lsiepel @jlaur
Update bundles/org.openhab.binding.tellstick/src/main/java/org/openha…
e83c197 
…b/binding/tellstick/internal/live/TelldusLiveDeviceController.java

Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: lsiepel <[email protected]>
@lsiepel
Merge branch 'openhab:main' into tellstick-null-annotations
b5baad7
@lsiepel
Fix review comments
d718cb6 
Signed-off-by: Leo Siepel <[email protected]>
@jlaur
Copy link
Contributor

jlaur commented Jan 6, 2025

@lsiepel - unfortunately I missed to merge this before New Year. There are now some conflicts to resolve, probably because of #18015.

@lsiepel
Copy link
Contributor Author

lsiepel commented Feb 19, 2025

@lsiepel - unfortunately I missed to merge this before New Year. There are now some conflicts to resolve, probably because of #18015.

Should be good now.

jlaur
jlaur requested changes Feb 19, 2025
View reviewed changes
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.

When checking changed files since my last review iteration, I found one bug. Other than that LGTM - I assume it's tested and ready to go?

lsiepel and others added 3 commits February 19, 2025 12:52
@lsiepel @jlaur
Update bundles/org.openhab.binding.tellstick/src/main/java/org/openha…
2605c8b 
…b/binding/tellstick/internal/live/TelldusLiveDeviceController.java

Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: lsiepel <[email protected]>
@lsiepel @jlaur
Update bundles/org.openhab.binding.tellstick/src/main/java/org/openha…
7f33244 
…b/binding/tellstick/internal/core/TelldusCoreBridgeHandler.java

Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: lsiepel <[email protected]>
@lsiepel
Fix review comment
df5622a 
Signed-off-by: Leo Siepel <[email protected]>
@lsiepel
Copy link
Contributor Author

lsiepel commented Feb 19, 2025

When checking changed files since my last review iteration, I found one bug. Other than that LGTM - I assume it's tested and ready to go?

No functionality has changed, i think it is ready to go, but i lack a tellstick so it was not very well tested.

jlaur
jlaur requested changes Feb 19, 2025
View reviewed changes
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.

Only one thing left from new commit.

...tick/src/main/java/org/openhab/binding/tellstick/internal/core/TelldusCoreBridgeHandler.java
eventHandler.addListener(this);
this.eventHandler = eventHandler;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed anymore, since already assigned in line 180.

Suggested change
this.eventHandler = eventHandler;

Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot about this when merging, but it's quite minor.

@jlaur
Copy link
Contributor

jlaur commented Feb 19, 2025

@jannegpriv, @ulfwin - I found some relatively recent PR's from you for this binding, perhaps you'd like to give this a test run if @lsiepel could do a 4.3.x build for production use?

@lsiepel
Copy link
Contributor Author

lsiepel commented Feb 19, 2025 •
edited
Loading

@jannegpriv, @ulfwin - I found some relatively recent PR's from you for this binding, perhaps you'd like to give this a test run if @lsiepel could do a 4.3.x build for production use?

Not sure if this is going to work, but give it a try:
https://1drv.ms/u/s!AnMcxmvEeupwj5MKc6wj8aRuFMVBZg?e=mlbFFi

@jlaur
Copy link
Contributor

jlaur commented Feb 19, 2025

Not sure if this is goping to work, but give it a try:

It probably would need to be cherry-picked on top of 4.3.x with the recent karaf upgrade etc. Let's wait and see if there will be a volunteer tester before going through this effort. 😉

@ulfwin
Copy link
Contributor

ulfwin commented Feb 19, 2025

I can give it a try, if I get some help. It seems to be a general change, so the fact that I'm not using the Telldus Live bridge is fine, right?

Also, I guess I need to update to the latest openhab? I would need to do that in a controlled manner, so it might take some extra time.

@jlaur
Copy link
Contributor

jlaur commented Feb 20, 2025

Also, I guess I need to update to the latest openhab?

Which version are you running now? It should be easy to create a JAR for 4.3.

jlaur
jlaur approved these changes Feb 23, 2025
View reviewed changes
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.

LGTM. Let's give it some mileage with M1 and in parallel see if we can have it tested with 4.3.

@jlaur jlaur merged commit 7224333 into openhab:main Feb 23, 2025
2 checks passed
@jlaur jlaur added this to the 5.0 milestone Feb 23, 2025
@lsiepel lsiepel deleted the tellstick-null-annotations branch February 23, 2025 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlaur jlaur jlaur approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@lsiepel @jlaur @ulfwin