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

Add switch entities for LCN key-locks and regulator-locks #127731

Merged
merged 10 commits into from
Oct 29, 2024

Conversation

alengwenus
Copy link
Contributor

@alengwenus alengwenus commented Oct 6, 2024

Proposed change

The LCN system allows for disabling key press events and regulator activities (so called locks). So far, binary sensors were implemented to monitor these lock states.
This PR implements two switch entity classes which not only allow to monitor the status, but also allow to switch the locks on and off.

The old binary sensor entity classes are marked deprecated.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

Comment on lines 88 to 102
async_create_issue(
self.hass,
DOMAIN,
"deprecated_regulatorlock_sensor",
breaks_in_ha_version="2025.2.0",
is_fixable=False,
is_persistent=False,
issue_domain=DOMAIN,
severity=IssueSeverity.WARNING,
translation_key="deprecated_regulatorlock_sensor",
translation_placeholders={
"domain": DOMAIN,
"integration_title": "LCN",
},
)
Copy link
Member

Choose a reason for hiding this comment

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

We should rather check if they are used in automations and scripts and raise if they are

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please give me a hint how to do that?

Would you also briefly explain to me, what's wrong about raising the issue at the moment when the entities are set up? Raising the issue should not only occur if the entities are used in automations and scripts but generally if they are used (even from the frontend)?

Copy link
Member

Choose a reason for hiding this comment

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

We usually get more false positives for users and we want issues to be actionable, not a "hey, did you know that ...".

In the past we deprecated the harmony switch entities, I think that's quite a clean example. (It's already removed from the codebase, but the PRs are still there)

I can link when I finally booted my pc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I guess I found it here: #119206.
I'll implement it in the same way.

@home-assistant home-assistant bot marked this pull request as draft October 26, 2024 05:50
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@alengwenus alengwenus marked this pull request as ready for review October 26, 2024 12:52
homeassistant/components/lcn/binary_sensor.py Outdated Show resolved Hide resolved
async_create_issue(
self.hass,
DOMAIN,
f"deprecated_binary_sensor_{self.entity_id}_{item}",
Copy link
Member

Choose a reason for hiding this comment

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

Let's not create an issue per item

@home-assistant home-assistant bot marked this pull request as draft October 29, 2024 13:31
@joostlek
Copy link
Member

Okay I just got the feedback from someone else that this was in the example I pointed at. Sorry for that :)

Co-authored-by: Joost Lekkerkerker <[email protected]>
@alengwenus
Copy link
Contributor Author

Okay I just got the feedback from someone else that this was in the example I pointed at. Sorry for that :)

So, should I leave it as it is?

@alengwenus alengwenus marked this pull request as ready for review October 29, 2024 14:38
@joostlek
Copy link
Member

I think we should remove the item, and I need to find a better example to point at haha

@joostlek joostlek merged commit c9aba28 into home-assistant:dev Oct 29, 2024
32 checks passed
@alengwenus
Copy link
Contributor Author

Unfortunately I did not entirely remove all placeholder keys in lcn/strings.json.
I fixed this in #129452 .

@alengwenus alengwenus deleted the dev-lcn-lock-switch branch October 29, 2024 19:32
@alengwenus alengwenus mentioned this pull request Oct 29, 2024
19 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Oct 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants