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

Support HassTurnOn/Off intents for lock domain #93231

Merged
merged 5 commits into from Nov 1, 2023

Conversation

tetele
Copy link
Contributor

@tetele tetele commented May 18, 2023

Breaking change

No

Proposed change

HassTurnOn/HassTurnOff intents were used for domains which had dedicated intents (e.g. cover - HassOpenCover). For the lock domain, there was no "translation" of intents to the proper lock services - lock.lock and lock.unlock.

This PR addresses this issue and fixes home-assistant/intents#1347

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 Black (black --fast 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.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @home-assistant/core, @synesthesiam, mind taking a look at this pull request as it has been labeled with an integration (intent) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of intent can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign intent Removes the current integration label and assignees on the pull request, add the integration domain after the command.

@tetele
Copy link
Contributor Author

tetele commented May 18, 2023

There were no tests for the similar but already handled issue for the cover domain. Should I add tests for both?

@synesthesiam
Copy link
Contributor

For binary sensor locks, they have:

lock: on means open (unlocked), off means closed (locked)

Shouldn't we follow suit here?

@tetele
Copy link
Contributor Author

tetele commented May 18, 2023

That's fair, but the intents are defined (and most likely translated) differently:

Let me know what you want to do

@tetele
Copy link
Contributor Author

tetele commented May 19, 2023

I think the best solution is to switch the contents and intent names of the 2 lock_HassTurnXx files, but I'm concerned about the tests in the intents repo. I don't think I want to tackle that issue myself :)

@tetele tetele marked this pull request as ready for review August 10, 2023 11:46
@tetele tetele requested review from synesthesiam and a team as code owners August 10, 2023 11:46
@clearwave1
Copy link

This was reported back in May and still is not fixed? Can someone please move this forward?

@Maxcodesthings
Copy link

@synesthesiam can you help move this forward? All checks are passing

@HNKNTA
Copy link

HNKNTA commented Nov 1, 2023

Hello everyone, please review and merge this, not having working locks really sucks.

@synesthesiam synesthesiam merged commit 66dd3b1 into home-assistant:dev Nov 1, 2023
21 checks passed
@synesthesiam
Copy link
Contributor

Thank you so much! Sorry for the delay.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Please address the comment in a new PR. Thanks!

hass.services.async_call(
LOCK_DOMAIN,
SERVICE_LOCK
if self.service == SERVICE_TURN_ON
Copy link
Member

Choose a reason for hiding this comment

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

Ternary operator expressions that span more than one line are really hard to read. I suggest defining a local variable above first and use that here.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 2, 2023
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.

Unexpected error with lock intent
6 participants