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

[danfossairunit] Remove deprecated channel main#manual_fan_speed when unlinked #11668

Merged

Conversation

jlaur
Copy link
Contributor

@jlaur jlaur commented Nov 29, 2021

Fixes #11667

Signed-off-by: Jacob Laursen jacob-github@vindvejr.dk

#11147 renamed a channel for consistency and left the old channel for backwards compatibility - "hidden" as an advanced channel.

This was a bit awkward. This pull request removes the deprecated channel if no items are linked. If any items are linked, a warning will be logged.

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur changed the title [danfossairunit] Provide more elegant deprecation of channel main#manual_fan_speed [danfossairunit] Remove deprecated channel main#manual_fan_speed if unlinked Nov 29, 2021
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur changed the title [danfossairunit] Remove deprecated channel main#manual_fan_speed if unlinked [danfossairunit] Remove deprecated channel main#manual_fan_speed when unlinked Nov 30, 2021
@jlaur
Copy link
Contributor Author

jlaur commented Dec 5, 2021

@pravussum - to avoid confusion, do you think it would be possible for you to review this in time for release 3.2? #11147 is also included in 3.2, but this PR makes the channel deprecation/migration a bit more elegant.

Copy link
Contributor

@pravussum pravussum 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
Copy link
Contributor Author

jlaur commented Dec 7, 2021

@lolodomo - as you approved previous #11147, maybe you could consider this also? It's a small change for improving the way old channel is deprecated. Previous PR was not part of 3.1, so it would be be less confusing for users if this could be included also with 3.2.

  • Before: Deprecated channel would be hidden as "Advanced", but would stay available all the time, even when no items are linked.
  • Now: Deprecated channel is not advanced, but only shown when any items are linked, and still with a clear deprecation warning.

@lolodomo
Copy link
Contributor

lolodomo commented Dec 7, 2021

I let another maintainer review in particular this change of channel type.
@cweitkamp : you know better than me what happened with the hue binding and users complaining about missing channels.

@jlaur
Copy link
Contributor Author

jlaur commented Dec 10, 2021

I let another maintainer review in particular this change of channel type. @cweitkamp : you know better than me what happened with the hue binding and users complaining about missing channels.

@cweitkamp - any comments?

@jlaur
Copy link
Contributor Author

jlaur commented Dec 12, 2021

@lolodomo - although I'm curious about the Hue binding and missing channels, please see my comment about the tests I've performed for this PR. After upgrading to this version, the deprecated channel will still work, but it will disappear from the UI until openHAB has been restarted. So for upgrading to 3.2 this should be fine.

@jlaur
Copy link
Contributor Author

jlaur commented Dec 17, 2021

@lolodomo - ?

@lolodomo
Copy link
Contributor

@jlaur : please look at my previous message. I prefer someone else decide what to do with your change. I have the feeling of a risk and I certainly do not want to take it myself just before a major release.

But I do not veto your proposal, I just don't know if this is a good approach or not.

@jlaur
Copy link
Contributor Author

jlaur commented Dec 17, 2021

@jlaur : please look at my previous message. I prefer someone else decide what to do with your change. I have the feeling of a risk and I certainly do not want to take it myself just before a major release.

Okay, didn't know if you had read my test documentation under the resolved code comment, posted 07.12.2021 16:56. IMHO it proves that it won't cause problems. Just wanted confirmation - if you read it, but don't accept - fine. If you missed it - just wanted to remind you. :-)

But I do not veto your proposal, I just don't know if this is a good approach or not.

Okay. The reason I'm gently pushing is that I'm not proud of the way I "renamed" the channel in #11147 (also new in 3.2), but didn't know better at the time. That PR left the old channel as a duplicate channel "hidden" as advanced. The new approach will completely remove it when it is no longer linked.

Until someone will pick up openhab/openhab-core#2590 or openhab/openhab-core#1924 it's a bit tedious to migrate channels.

@kaikreuzer - would you be able to assess? Currently seems like we don't know who can give a second opinion as @cweitkamp hasn't responded.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

If you have thoroughly tested it, let's squeeze this in.
You are right, without having support in the core for automatic migration, every solution is awkward...
It would be great if you could do a PR against https://github.com/openhab/openhab-distro/blob/main/distributions/openhab/src/main/resources/bin/update.lst#L46-L50 to add this as a (almost) breaking change as well.

@kaikreuzer kaikreuzer merged commit 4eea4ac into openhab:main Dec 17, 2021
@kaikreuzer kaikreuzer added this to the 3.2 milestone Dec 17, 2021
@jlaur
Copy link
Contributor Author

jlaur commented Dec 17, 2021

@kaikreuzer - thanks. Created openhab/openhab-distro#1351 with this note as well as notes for some other bindings I've worked on.

NickWaterton pushed a commit to NickWaterton/openhab-addons that referenced this pull request Dec 30, 2021
… unlinked (openhab#11668)

* Remove deprecated channel if unlinked.

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Nick Waterton <n.waterton@outlook.com>
mischmidt83 pushed a commit to mischmidt83/openhab-addons that referenced this pull request Jan 9, 2022
… unlinked (openhab#11668)

* Remove deprecated channel if unlinked.

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Michael Schmidt <mi.schmidt.83@gmail.com>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Jan 28, 2022
… unlinked (openhab#11668)

* Remove deprecated channel if unlinked.

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
… unlinked (openhab#11668)

* Remove deprecated channel if unlinked.

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur deleted the 11667-danfossairunit-channel-deprecation branch June 13, 2022 18:51
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
… unlinked (openhab#11668)

* Remove deprecated channel if unlinked.

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
… unlinked (openhab#11668)

* Remove deprecated channel if unlinked.

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[danfossairunit] Provide more elegant deprecation of channel main#manual_fan_speed
4 participants