-
Notifications
You must be signed in to change notification settings - Fork 526
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
[FR] Expand timer naming words #2515
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
@bors-ltd Is the PR ready for review? |
Sorry, I was so excited when I found out Home Assistant had voice commands, I got carried away. It reminded me of Snips, glad I kept the devkit. Only later I noticed the test suite, and realised my changes were not covered. But I got busy today and couldn't come back to it. I'll ping you when it's thoroughly tested, and hopefully on real hardware when my ESP32 is delivered. |
d34a7e9
to
b8ab1db
Compare
Now I feel like pretty happy with the state of that PR, and confident to ask for a review. Please note I'm new to HA, so it may likely not meet your quality standards, and I can accept criticism, even nitpicking. Hopefully I didn't reach any hard limit with Whisper's current capabilities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the very good job ! I just have a small comment about the implementation but overall it's pretty good 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we decided to have "Il reste combien" and "Combien de temps" but not "Combien" alone as it's too generic. Having one of the vert "reste" or the word "temps" must be required to avoid interference with other intents. "Combien" alone should be avoided.
@bors-ltd Thx a lot for your PR and sorry for the delay reviewing it on my side. My review is done, it's basically 3 small changes
|
Thanks for the review, I'll be implementing the changes ASAP. |
Updated my PR with all the suggestions/corrections. I still have to test it in real life situations with my new buddy the ESP32-S3-Box-3 (which proved to be an even deeper rabbit hole...), so consider it a draft for now. I'll keep the "Combien pour la pizza" on my personal HA, because I don't think I'll ever ask HA it the price of what I cook. 😜 |
I find it more natural to say "start a 5 minutes timer for the pizza", or "how much left for the pizza". Also fixed a few typos.
I find it more natural to say "start a 5 minutes timer for the pizza".
The "surnommé" typo isn't strictly necessary, but it was bothering me.