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

[mideaac] Initial Contribution #17395

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

apella12
Copy link
Contributor

@apella12 apella12 commented Sep 9, 2024

new midea ac binding - Had to delete old one due to rebase malfunction (operator error)
Signed-off-by: Bob Eckhoff [email protected]

I’d like to put this out for new binding review.
This binding was started by the original author as a forum topic, but has since had other contributors. I picked it up when I installed a mini-split AC that used the Midea protocol earlier this year, originally to allow for longer polling frequencies. Then as a retirement challenge (no formal java training), decided to attempt to clean it up for official status. I cleaned up the summary report from over 150 items and believe I conformed to current developer guidelines (Java docs, naming, tests, etc.). Along the way added some additional functionality and corrected some code issues.
The discovery and security classes remain largely unchanged from the original author (who I haven’t been able to contact), and work well. The other classes I understand pretty well at this point and hopefully can address any concerns.
As a note, to conform to the guidelines there are breaking changes in naming from the version I published on the forum thread.

new midea ac binding - Had to delete old one due to rebase malfunction (operator error)
Signed-off-by: Bob Eckhoff <[email protected]>
@apella12 apella12 requested a review from a team as a code owner September 9, 2024 21:42
@lsiepel
Copy link
Contributor

lsiepel commented Sep 10, 2024

Thank you for creating this PR to contribute this binding.
To manage expectations; review capacity is somewhat limited so don’t expect an immediate full review. To speed up the process you can already perform a self-review by checking this list: https://github.com/openhab/openhab-addons/wiki/Review-Checklist

Have not looked at this PR yet, it may be of very good quality. But common review comments for new bindings are about thing structure (xml), naming convention and log levels. So you might double check those areas.

Conform to OH logging on Thing offline status changes, review of log levels.
Signed-off-by: Bob Eckhoff <[email protected]>
@apella12 apella12 marked this pull request as draft September 12, 2024 15:10
Added Javadoc to public methods excluding enum lists and binding constants

Signed-off-by: Bob Eckhoff <[email protected]>
@apella12 apella12 marked this pull request as ready for review September 13, 2024 20:07
Add body type filter to ensure meaningful readings.
Signed-off-by: Bob Eckhoff <[email protected]>
@lolodomo lolodomo added the new binding If someone has started to work on a binding. For a new binding PR. label Sep 18, 2024
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.

3 participants