search

MycroftAI / skill-homeassistant

Mycroft Skill/Integration for Homeassistant
GNU Lesser General Public License v3.0
114 stars 62 forks source link

Add padatious intents for turn on, turn off and toggle actions. #36

Closed kamir86 closed 3 years ago

kamir86 commented 4 years ago

Description:

Add padatious intents for several actions. Language supported: english, italian, czech and geman, thanks to @Tony763 and @Manasovo . This fixes several intents, that're not working on 20.02 in several languages outside english. See my first comment: on issue #25

Changes:

Related issue (if applicable): fixes #25 Italian english czech and geman, thanks to @Tony763 and @Manasovo , I need some support for other languages

Checklist:

kamir86 commented 4 years ago

Hello, thanks for your reply. I think that a popular issue should not remain unfixed for a maybe future development in the future. Mycroft will eventually support conversational contexts in padatious intents too. I'm not in the development team but it's clearly stated in the skill guide. I'm open to stick with adapt intent to fix this, but the fix won't be easy and a lot of language specific code should be added, mind of the current de-de support.

stratus-ss commented 4 years ago

I think that a popular issue should not remain unfixed for a maybe future development in the future.

This is a fair point. I like using Padatious intents for their simplicity and I can see why you would want to do this for additional language support.

I'm open to stick with adapt intent to fix this, but the fix won't be easy and a lot of language specific code should be added, mind of the current de-de support.

With this in mind I think your proposal has merit. It will pend testing and PR #41 but other than that I think the idea is sound and your rationale is good. Thanks for the fix. Someone will try to get this tested soon. I'm going to try and be more on top of this repo going forward

stratus-ss commented 3 years ago

@kamir86 PR #41 was approved and merged. I came back to look at this one and see there is a conflict now. Would you mind seeing if you can address the conflict?

Thanks

kamir86 commented 3 years ago

Hello, I'll check this as soon as i can

stratus-ss commented 3 years ago

Thanks for the update. I'll be ready to try this, this week. I've looked over the the diffs. Its just a matter of testing the changes.

Do you have sample sentences I can try with the expected results.

I'm sure I can figure it out from the diffs, but it would be helpful to have the phrases you tested with

kamir86 commented 3 years ago

Hello thanks for merging this. Sorry for the late response, i can provide some italian test sentences if you want to integrate your tests: Turn on intent: Accendi la luce della sala. Accendi l'interruttore dello studio. Turn off intent: Spegni la luce della sala. Spegni l'interruttore dello studio. Toggle intent: Commuta la luce della sala. Commuta l'interruttore dello studio. Increase brigthness intent: Alza la luminosità della sala. Rendi lo studio più luminoso. Dec brigthness intent: Abbassa la luminosità della sala. Rendi lo studio meno luminoso.

Tony763 commented 3 years ago

Hi @kamir86 as I know tests are possible only in English. I waiting for merging all my PRs as Tests are already prepared (in English), also I have prepared a Travis testing solution.
@stratus-ss is there a plan for adding support for other languages in Voight Kampff tests engine?

kamir86 commented 3 years ago

hello @Tony763 thanks for adding english tests. It would be great to have tests in other languages ​​as well, this would prevent changes from producing defects, like the one this PR was addressed

krisgesling commented 3 years ago

@Tony763 @kamir86 - the issue for exploring this is at: https://github.com/MycroftAI/mycroft-core/issues/2552