-
Notifications
You must be signed in to change notification settings - Fork 632
[SV] Fix #2944 - Response is None #2955
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Tudor Sandu <tm.sandu@gmail.com>
Co-authored-by: Tudor Sandu <tm.sandu@gmail.com>
larsdunemark
left a comment
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.
I think that you need to base your commits on the upstream main branch to avoid the problem where you get this merge problem with the README.md, in this PR it would remove the README.md from the main project if merged.
Please also try to make the changes smaller to avoid changing multiple domains as once if possible. This change both the way we handles lights and covers.
| - sentences: | ||
| - (regla | <stäng_gardiner>) garagedörren | ||
| response: cover_device_class | ||
| response: cover |
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.
This should still response Öppande garaget (depending on the state of translation it might give the device_class in that case we might need to handle the translation also.
| cover: "Öppnade {{ slots.name }}" | ||
| cover: "Öppnade" | ||
| cover_area: "Öppnade i {{ slots.area }}" | ||
| cover_device_class: "Öppnade {{ slots.device_class }}" |
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.
This is not the correct change, we don't want the response for a device_class to not include the class.
The intention it that this will say
Öppnande garaget if a command was issued for a device that will open that device_class.
| - sentences: | ||
| - <öppna_gardiner> garagedörren | ||
| response: cover_device_class | ||
| response: cover |
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.
Same here.
| - sentences: | ||
| - <stäng_gardiner> <name> #close curtains | ||
| requires_context: | ||
| domain: | ||
| - cover |
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.
Should probably be moved to the cover_HassTurnOff file
| - sentences: | ||
| - <slå_av> <name> # lights off | ||
| requires_context: | ||
| domain: | ||
| - light | ||
| response: light |
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.
Moving to light_HassTurnOff file.
This will have the same sentence as the first but with a respond.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| - cover | ||
| response: cover | ||
| - sentences: | ||
| - <slå_av> <name> # lights off |
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.
I think that one issue can be that the expansion rule for <slå_av> is a bit to wide, in contains both the work turn off, stopp, shutoff etc in swedish and for lights it might make more since to split it up in multiple sentences.
like:
"släck "
looking at the DA language.
Excluded light domain from default and added separate response for light. I believe this could fix #2944