Skip to content

[broadlinkthermostat] Aesthetic rename and add RM Mini - #13412

Merged
lolodomo merged 3 commits into
openhab:mainfrom
GiviMAD:broadlink/migrate_binding
Oct 16, 2022
Merged

[broadlinkthermostat] Aesthetic rename and add RM Mini #13412
lolodomo merged 3 commits into
openhab:mainfrom
GiviMAD:broadlink/migrate_binding

Conversation

@GiviMAD

@GiviMAD GiviMAD commented Sep 18, 2022

Copy link
Copy Markdown
Member

Signed-off-by: Miguel Álvarez miguelwork92@gmail.com

This is a proposal for renaming the broadlinkthermostat binding into broadlink also add a new device type "RM Mini" which is a universal controller infrared device.
Instead of adding a new binding I thought it was better to rename the current one, as the package relies in a library which supports other devices, as indicated in the readme.
Let me know in case is better to create a new binding.
To keep the autor aware @flo-02-mu.

@GiviMAD
GiviMAD requested a review from a team as a code owner September 18, 2022 21:20
@GiviMAD
GiviMAD requested review from lolodomo and lsiepel September 18, 2022 21:22
@wborn wborn added enhancement An enhancement or new feature for an existing add-on (potentially) not backward compatible labels Sep 20, 2022
@GiviMAD

GiviMAD commented Oct 1, 2022

Copy link
Copy Markdown
Member Author

CI error seems to be unrelated.

@lolodomo lolodomo added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Oct 1, 2022
@lolodomo

lolodomo commented Oct 2, 2022

Copy link
Copy Markdown
Contributor

Let me know in case is better to create a new binding.

I have no definitive opinion on the two possible options.
I would be in favour of your proposal but of course this will be a breaking change for any user who previously installed this binding. At the same time, this is probably not a very used binding ?

@openhab/add-ons-maintainers : WDYT ?

@fwolter

fwolter commented Oct 2, 2022

Copy link
Copy Markdown
Member

I think "broadlink" would be a better name, too. We discussed this partially in #11049. But I don't see much benefit of renaming this now and breaking all installations. I'd suggest to rename it with the OH 4 release (whenever that will be) and add the new device in the existing binding.

@GiviMAD

GiviMAD commented Oct 2, 2022

Copy link
Copy Markdown
Member Author

@lolodomo, @fwolter thanks for the feedback. I will revert the package id while keeping other changes that will not break the binding.
I will put this on hold a couple of days while finishing another contribution.

Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@GiviMAD
GiviMAD force-pushed the broadlink/migrate_binding branch from b83c04f to 10b9a3a Compare October 7, 2022 15:41
@GiviMAD GiviMAD changed the title [broadlink] Rename from broadlinkthermostat and add RM Mini [broadlinkthermostat] Aesthetic rename and add RM Mini Oct 7, 2022
@GiviMAD

GiviMAD commented Oct 7, 2022

Copy link
Copy Markdown
Member Author

@lolodomo, @fwolter I have reverted the package name changes while keeping the name changes and the code changes.
Do you think I should add a warning on the readme about the package name been different than the binding name?

Comment thread bundles/org.openhab.binding.broadlinkthermostat/README.md Outdated
@lolodomo

Copy link
Copy Markdown
Contributor

As mentioned in one of my review comments, you introduced a breaking change (by renaming a thing type) and I think it can be avoided, in a simple way.

Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@GiviMAD
GiviMAD requested a review from lolodomo October 15, 2022 21:59
@lolodomo

Copy link
Copy Markdown
Contributor

There is now no more breaking changes that I can identify, that is a good point.

@GiviMAD
GiviMAD requested a review from lolodomo October 16, 2022 14:51
Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>

@lolodomo lolodomo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thank you

@lolodomo
lolodomo merged commit 5762d23 into openhab:main Oct 16, 2022
@lolodomo lolodomo added this to the 3.4 milestone Oct 16, 2022
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
* [broadlinkthermostat] Aesthetic rename and add RM Mini

Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
* [broadlinkthermostat] Aesthetic rename and add RM Mini

Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
@openhab-bot

Copy link
Copy Markdown
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/broadlink-binding-for-rmx-a1-spx-and-mp-any-interest/22768/1527

borazslo pushed a commit to borazslo/openhab-mideaac-addon that referenced this pull request Jan 8, 2023
* [broadlinkthermostat] Aesthetic rename and add RM Mini

Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
* [broadlinkthermostat] Aesthetic rename and add RM Mini

Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
* [broadlinkthermostat] Aesthetic rename and add RM Mini

Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement An enhancement or new feature for an existing add-on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants