Deprecate Wink #54406
Deprecate Wink #54406
Conversation
probot-home-assistant
bot
added
code-quality
integration: wink
small-pr
|
Another point of view: Wink is dead. |
janiversen
changed the title
|
Followed your idea and changed the PR to deprecate Wink. |
There was a problem hiding this comment.
- No need to deprecate each platform, just the integration is enough
- Deprecate the config schema of the integration
- Adjust documentation for the deprecation warning
- The text in the breaking change seems odd, it has been maintained. Please add the actual reason for the removal of Wink itself so it can be used in the release notes.
Why deprecated in 2021.12? We generally handle 2 cycles, which means 2021.9 its shipped, so can be removed in 2021.11. Any specific reason to keep it around 3 cycles?
| _LOGGER.warning( | ||
| "The Wink integration has been deprecated and will be removed in " | ||
| "Home Assistant Core 2021.12" | ||
| ) |
There was a problem hiding this comment.
This is an integration... why throw a deprecation warning on every platform instead of doing it centrally?
There was a problem hiding this comment.
Please revert the platform warnings
There was a problem hiding this comment.
Does that really matter ? this is going to be in for 3 months and then be deleted. Why use more time on this as necessary ?
It would be nice if could use time on items that really matter, instead of perfecting a PR that just schedules deletion, but that is just my opinion.
If it really disturbs you can always do a followup PR.
There was a problem hiding this comment.
It now spams the logs for existing users and this is not how we deprecate integrations in general.
Please revert the platform warnings.
There was a problem hiding this comment.
There are 7+ active users, and I tried to ensure they got an informative warning, whenever they started HA, that is in my mind being user friendly.
I implemented the change in schema you demanded, which is an anonymous message leading to confusion.
In Lyft you did what I did, you did not block the schema, but gave the users an informative message….but you deny me the same possibility.
Please lets end this bad discussion, and get the PR merged, so it can be forgotten.
There was a problem hiding this comment.
In Lyft you did what I did, you did not block the schema
Lyft is a platform integration, which does not have a generic integration setup.
But you are now making me run into circles.
Please lets end this bad discussion, and get the PR merged, so it can be forgotten.
?
I'm not sure what you are looking for, but please adjust this PR into how we generally handle this. If not, I'm not going to approve this PR.
|
The reason for 3 cycles is because that is what I have been told when I deprecated features in modbus, but that is something I can change. @balloob wrote in discord “ If this is about Wink, that box has been unmaintained for a long time” and you tell me it has been maintained, sorry I will not be in the middle of that discussion. I choose to deprecate all entities, just as you did with lyft who also have a schema (which you did not deprecate) https://github.com/home-assistant/core/pull/53005/files If its important to you I can also add it to the schema. I have added documentation which I copied from a PR of yours. Seems you do not agree with paulus who approved the PR, I am (as usual) prepared to make any changes requested but to a limit. I get the sense that the real theme is something else, so for the sake of peace feel free to close this PR and merge your own. |
|
Lyft is just a platform, there is nothing else to deprecate. This is full integration, we deprecate that differently compared to a platform integration like Lyft. There is no need for each platform in this integration to throw up a warning in the logs if it can be done on an integration level just once (and only needs a single warning to implement as well). This is why the config schema (the domain) can be marked deprecated as well in this case (which we do normally as well). |
|
Well you could have deprecated the schema instead of doing it in setup_platform, there are really no logic in doing it differently. But I will add deprecate to the schema. |
| _LOGGER.warning( | ||
| "The Wink integration has been deprecated and will be removed in " | ||
| "Home Assistant Core 2021.12" | ||
| ) |
There was a problem hiding this comment.
Please revert the platform warnings
|
Enough is enough, you win, you managed to make me close this to get peace! The PR was approved, and not by just anyone, but you obviously had contradicting demands. I am just sorry, I let it sit for hours after approval instead of just merging it when approved and without CI errors, having done that would have saved a lot of wasted time. @balloob sorry to have wasted your approval, this is the second of approx 200 PRs, where the review only manages to demotivate and not helps getting HA better or more stable. |
|
🤷 It is a fairly simple change that can be implemented a lot cleaner, smaller and more close to we normally do it. But fine, as you wish 👍 |
|
Replacement PR #54496 |
Breaking change
Scheduling Wink to be removed in release 2021.12, as the box have not been maintained for years.
Proposed change
Type of change
Additional information
home-assistant/home-assistant.io#18884
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: