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

[DE] Add binary sensor light #1931

Merged
merged 4 commits into from
Feb 16, 2024
Merged

[DE] Add binary sensor light #1931

merged 4 commits into from
Feb 16, 2024

Conversation

steffenrapp
Copy link
Contributor

No description provided.

@steffenrapp steffenrapp marked this pull request as ready for review February 4, 2024 18:29
Copy link
Contributor

@mib1185 mib1185 left a comment

Choose a reason for hiding this comment

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

Hi @steffenrapp
thanks for this ... even sentences like "wie viel Licht ist an?" or "ist irgendwo Licht ausgelöst?" sounds weird to me 🙈
we still need some more test sentences, since we add 15 sentences, but only 12 test sentences ... also they use many expansion rules and optional words.
maybe we should create two new expansion rules:

ist_wurde: "(ist|wurde)"
sind_wurden: "(sind|wurden)"

than replace (ist|wurde) by <ist_wurde> and (ist|sind|wurde[n]) by (<ist_wurde>|<sind_wurden>) ... with this the amount of test sentences can be reduced, since we do not need to check all these optionals multiple times

@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.

@home-assistant home-assistant bot marked this pull request as draft February 11, 2024 22:19
@steffenrapp
Copy link
Contributor Author

Hey @mib1185 thank you for the feedback. I know, not every combination sounds natural. I think if it doesn't do anything wrong it shouldn't be a problem. Many voice assistants understand sentences that are not grammatically correct.
I've implemented those expansion rules (may use them at more places in a separate PR) and added more test sentences with area to cover them.

@steffenrapp steffenrapp marked this pull request as ready for review February 16, 2024 21:51
Copy link
Contributor

@mib1185 mib1185 left a comment

Choose a reason for hiding this comment

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

thanks @steffenrapp 👍 i think this is ok for now

@mib1185 mib1185 merged commit 5844e8d into home-assistant:main Feb 16, 2024
2 checks passed
@steffenrapp steffenrapp deleted the de-binary-sensor-1 branch February 17, 2024 11:31
schizza pushed a commit to schizza/intents that referenced this pull request Mar 16, 2024
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.

2 participants