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

[bigassfan] Add null annotations #13903

Merged
merged 13 commits into from
Jan 5, 2023
Merged

[bigassfan] Add null annotations #13903

merged 13 commits into from
Jan 5, 2023

Conversation

lsiepel
Copy link
Contributor

@lsiepel lsiepel commented Dec 10, 2022

Signed-off-by: Leo Siepel [email protected]

Changes:
added null annotations,
minor refactoring
fixed all SAT warnings

Edit: 4.0.0 JAR is available here: https://1drv.ms/u/s!AnMcxmvEeupwjp4Kbj6wM5O_dnnDew?e=z2G4qO

@lsiepel lsiepel requested a review from mhilbush as a code owner December 10, 2022 19:41

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Signed-off-by: Leo Siepel <[email protected]>
Signed-off-by: Leo Siepel <[email protected]>
@lsiepel lsiepel requested a review from wborn December 23, 2022 12:50
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 the refactoring. I have provided some feedback.

@lsiepel lsiepel requested a review from jlaur December 26, 2022 18:16
Signed-off-by: lsiepel <[email protected]>
Signed-off-by: lsiepel <[email protected]>
@lsiepel
Copy link
Contributor Author

lsiepel commented Jan 2, 2023

All comments have been adressed, thanks.

@jlaur
Copy link
Contributor

jlaur commented Jan 2, 2023

All comments have been adressed, thanks.

So just one final confirmation - since not labelled "additional testing preferred", I assume you have given it a good spin yourself 🙂?

@mhilbush, you are code owner, would you like to have a look before merging?

@lsiepel
Copy link
Contributor Author

lsiepel commented Jan 2, 2023

No i don't have this device, it is just from technicall code perspective. As it isn't that super complex, i'm confident we dont have any regressions like the bosesoundtouch. Yeah, i learned from that ;-)

Also with 4.0 release far away any regressions can be fixed on time before a release, and besides snapshots being unstable at the moment i would not count on anyone testing at the moment. I leave it to you or @mhilbush

@jlaur
Copy link
Contributor

jlaur commented Jan 2, 2023

No i don't have this device, it is just from technicall code perspective. As it isn't that super complex, i'm confident we dont have any regressions like the bosesoundtouch. Yeah, i learned from that ;-)

We usually assume that the code has been tested, so thanks for the honesty here. My general advise would be in line with @fwolter's here: #14023 (comment).

Although I agree that the risk is low, there is a real non-zero risk of breaking something which worked fine (and was tested by many users) without null annotations. See for example #13895 (comment). With ~200 lines changed, we both could have missed something. I recently also did this in #14010 with less confidence, even though just two lines changed, but in this case we had more to win than we had to lose.

Summing up, my general advise would be to prioritize bindings which you can either test yourself, or with backup from someone who can test. If untested, please clearly state this in the description and also use the label "Additional testing preferred". This can give a hint to reviewers to take more care, and could also trigger some coordination to possibly have someone help testing it.

Yet, here we are. I don't see any problems which were not already mentioned, but would like input from @mhilbush. If the JAR is up-to-date, perhaps we can look for a tester in previous PR's/issues for this binding or in the community?

Sorry for being overly cautious, but better safe than sorry. 😄 I don't think we need to rush it.

@jlaur jlaur added the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Jan 2, 2023
@lsiepel
Copy link
Contributor Author

lsiepel commented Jan 2, 2023

No i don't have this device, it is just from technicall code perspective. As it isn't that super complex, i'm confident we dont have any regressions like the bosesoundtouch. Yeah, i learned from that ;-)

We usually assume that the code has been tested, so thanks for the honesty here. My general advise would be in line with @fwolter's here: #14023 (comment).

Although I agree that the risk is low, there is a real non-zero risk of breaking something which worked fine (and was tested by many users) without null annotations. See for example #13895 (comment). With ~200 lines changed, we both could have missed something. I recently also did this in #14010 with less confidence, even though just two lines changed, but in this case we had more to win than we had to lose.

Summing up, my general advise would be to prioritize bindings which you can either test yourself, or with backup from someone who can test. If untested, please clearly state this in the description and also use the label "Additional testing preferred". This can give a hint to reviewers to take more care, and could also trigger some coordination to possibly have someone help testing it.

Yet, here we are. I don't see any problems which were not already mentioned, but would like input from @mhilbush. If the JAR is up-to-date, perhaps we can look for a tester in previous PR's/issues for this binding or in the community?

Sorry for being overly cautious, but better safe than sorry. 😄 I don't think we need to rush it.

I understand your position and do take your and fwolter's advice serieus.. Couple of weeks ago i started to fix SAT issues to get to know more about openHAB's ecosystem and be of any value. And as you might have seen, after some of those PR's i stopped and switched to bugs/enhancements. While this and some PR's where allready there, i hope they are of added value and get merged and if not so be it.

@mhilbush
Copy link
Contributor

mhilbush commented Jan 2, 2023 via email

@mhilbush
Copy link
Contributor

mhilbush commented Jan 2, 2023

So if you could hold off on merging I would appreciate it.

@mhilbush mhilbush self-assigned this Jan 3, 2023
@mhilbush
Copy link
Contributor

mhilbush commented Jan 3, 2023

@jlaur Thank you for the time you invested in the review so far.

@mhilbush
Copy link
Contributor

mhilbush commented Jan 4, 2023

Built and installed the binding, and the handlers won't start.

2023-01-04 13:18:55.410 [DEBUG] [inding.bigassfan.internal.handler.BigAssFanHandler] - BigAssFanHandler for bigassfan:fan:----- is initializing
2023-01-04 13:18:55.410 [DEBUG] [inding.bigassfan.internal.handler.BigAssFanHandler] - BigAssFanHandler config for bigassfan:fan:----- is BigAssFanConfig{label=Master Bedroom Fan, ipAddress=---, macAddress=---}
2023-01-04 13:18:55.410 [DEBUG] [inding.bigassfan.internal.handler.BigAssFanHandler] - BigAssFanHandler config of bigassfan:fan:--- is invalid. Check configuration

Signed-off-by: lsiepel <[email protected]>
Signed-off-by: lsiepel <[email protected]>
@mhilbush
Copy link
Contributor

mhilbush commented Jan 4, 2023

Thanks. Handlers have started successfully.

I will continue to run for a while in my environment.

Signed-off-by: lsiepel <[email protected]>
Copy link
Contributor

@mhilbush mhilbush left a comment

Choose a reason for hiding this comment

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

I only have a few comments. Might have some more when I do a second pass.

Signed-off-by: lsiepel <[email protected]>
Signed-off-by: lsiepel <[email protected]>
Copy link
Contributor

@mhilbush mhilbush left a comment

Choose a reason for hiding this comment

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

This has been running well for me. And I don't have any other changes or comments. Therefore I'm good with merging.

LGTM

@jlaur jlaur removed the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Jan 5, 2023
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

@jlaur jlaur merged commit cb46065 into openhab:main Jan 5, 2023
@jlaur jlaur added this to the 4.0 milestone Jan 5, 2023
@jlaur jlaur changed the title [bigassfan] Null annotations [bigassfan] Add null annotations Jan 5, 2023
@lsiepel lsiepel deleted the bigassfan-null branch January 5, 2023 22:22
borazslo pushed a commit to borazslo/openhab-mideaac-addon that referenced this pull request Jan 8, 2023
* Null annotations and some refactoring
* Fix synchronized block
* Fix remaining warnings

Signed-off-by: Leo Siepel <[email protected]>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
* Null annotations and some refactoring
* Fix synchronized block
* Fix remaining warnings

Signed-off-by: Leo Siepel <[email protected]>
renescherer pushed a commit to renescherer/openhab-addons that referenced this pull request Mar 23, 2023
* Null annotations and some refactoring
* Fix synchronized block
* Fix remaining warnings

Signed-off-by: Leo Siepel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants