[EN] Enhanced get state sentences for media_player
#2230
Conversation
This is used for confirming ambiguous state values which could be rephrased/reinterpreted
Passing the `state` slot as a list to the parser/test processor, although supported in hassil, would have not worked.
WalkthroughWalkthroughThis update enhances the Home Assistant's Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Assistant
participant StateHandler
participant ResponseHandler
User->>Assistant: Is the receiver on?
Assistant->>StateHandler: Query State of Media Player
StateHandler->>Assistant: State: playing
Assistant->>ResponseHandler: Format Response with State
ResponseHandler->>Assistant: Receiver is on and playing
Assistant->>User: Receiver is on and playing
Assessment Against Linked Issues
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (6)
Additional context usedPath-based instructions (1)
GitHub Check: build
Ruff
Additional comments not posted (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| actual_state: "on" | ||
| alternate_state: | ||
| "on": "in an unknown state" | ||
| idle: "not playing" | ||
| standby: "in standby" |
There was a problem hiding this comment.
Maybe these should actually be metadata?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range comments (4)
shared/__init__.py (4)
Line range hint
120-168: The changes toget_matched_statesenhance flexibility by allowingstate_nameto accept both strings and lists. However, the function's complexity is noted as too high. Consider refactoring to simplify the logic.- def get_matched_states( + def get_matched_states( # Consider breaking down into smaller helper functions or simplifying the conditional logic.Tools
Ruff
166-169: Combine
ifbranches using logicaloroperator (SIM114)Combine
ifbranches
Line range hint
179-179: Theget_matched_timersfunction is noted to be slightly too complex. Consider refactoring to simplify the conditional logic or breaking it into smaller functions.
Line range hint
242-242: Consider enablingautoescapein Jinja2 to mitigate potential XSS vulnerabilities.- return Environment(loader=BaseLoader(), undefined=StrictUndefined) + return Environment(loader=BaseLoader(), undefined=StrictUndefined, autoescape=select_autoescape(['html', 'xml']))
Line range hint
254-254: Avoid usingassertin production code as it can be disabled in optimized runs. Use explicit error handling instead.- assert response_template, "Empty response template" + if not response_template: + raise ValueError("Empty response template")
| alternate_state: | | ||
| {{ slots.name | capitalize }} is {{ slots.actual_state }}, but it is | ||
| {% if state.state in slots.alternate_state %} | ||
| {{ slots.alternate_state[state.state] }} | ||
| {% else %} | ||
| {{ state.state }} | ||
| {% endif %} |
There was a problem hiding this comment.
The new alternate_state response template enhances descriptive accuracy but may have an issue with the actual_state attribute.
- {{ slots.name | capitalize }} is {{ slots.actual_state }}, but it is
+ {{ slots.name | capitalize }} is {{ state.state_with_unit }}, but it isCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| alternate_state: | | |
| {{ slots.name | capitalize }} is {{ slots.actual_state }}, but it is | |
| {% if state.state in slots.alternate_state %} | |
| {{ slots.alternate_state[state.state] }} | |
| {% else %} | |
| {{ state.state }} | |
| {% endif %} | |
| alternate_state: | | |
| {{ slots.name | capitalize }} is {{ state.state_with_unit }}, but it is | |
| {% if state.state in slots.alternate_state %} | |
| {{ slots.alternate_state[state.state] }} | |
| {% else %} | |
| {{ state.state }} | |
| {% endif %} |
There was a problem hiding this comment.
That's a slot being passed from sentences/en/media_player_HassGetState.yaml, so first of all it's not an attribute and second of all it is not a problem.
There was a problem hiding this comment.
@tetele Thank you for the clarification on the actual_state slot usage. I appreciate your insight and understanding of the implementation details. If there's anything else you need assistance with regarding this PR, feel free to let me know!
| - "on" | ||
| - "idle" | ||
| - "paused" | ||
| - "standby" | ||
| - "buffering" | ||
| actual_state: "not off" | ||
| alternate_state: | ||
| "on": "in an unknown state" | ||
| idle: "not playing anything" | ||
| standby: "in standby" | ||
| context: | ||
| domain: media_player | ||
| volume_level: "50" | ||
| response: "Tv is not off, but it is not playing anything" | ||
|
|
||
| - sentences: | ||
| - "is the TV playing?" | ||
| intent: | ||
| name: HassGetState | ||
| slots: | ||
| name: "TV" | ||
| state: "playing" | ||
| context: | ||
| domain: media_player | ||
| volume_level: "50" | ||
| response: "No, idle" | ||
|
|
||
| - sentences: | ||
| - "is the TV standing by?" | ||
| intent: | ||
| name: HassGetState | ||
| slots: | ||
| name: "TV" | ||
| state: | ||
| - "on" | ||
| - "idle" | ||
| - "standby" | ||
| - "buffering" | ||
| context: | ||
| domain: media_player | ||
| volume_level: "50" | ||
| response: "Yes" | ||
|
|
||
| - sentences: | ||
| - "are any media players on in the kitchen?" | ||
| intent: | ||
| name: HassGetState | ||
| slots: | ||
| area: "Kitchen" | ||
| domain: "media_player" | ||
| state: "playing" | ||
| response: "No" | ||
|
|
||
| - sentences: | ||
| - "are all media players on?" | ||
| intent: | ||
| name: HassGetState | ||
| slots: | ||
| domain: "media_player" | ||
| state: "playing" | ||
| response: "No, TV is not on" | ||
|
|
||
| - sentences: | ||
| - "are all media players off?" | ||
| intent: | ||
| name: HassGetState | ||
| slots: | ||
| domain: "media_player" | ||
| state: "off" | ||
| response: "No, TV is not off" | ||
|
|
||
| - sentences: | ||
| - "which media players are on?" | ||
| intent: | ||
| name: HassGetState | ||
| slots: | ||
| domain: "media_player" | ||
| state: "playing" | ||
| response: "Not any" | ||
|
|
||
| - sentences: | ||
| - "how many media players are on?" | ||
| intent: | ||
| name: HassGetState | ||
| slots: | ||
| domain: "media_player" | ||
| state: "playing" | ||
| response: "0" | ||
|
|
||
| - sentences: | ||
| - "are all the media players on in the living room" | ||
| - "are all media players on in the living room?" | ||
| - "are all media players in the living room on?" | ||
| intent: | ||
| name: HassGetState | ||
| slots: | ||
| domain: "media_player" | ||
| state: "playing" | ||
| area: "Living Room" | ||
| response: "No, TV is not on" |
There was a problem hiding this comment.
The test scenarios are well-designed but there's an issue with the format of alternate_state.
- alternate_state:
- "on": "in an unknown state"
- idle: "not playing"
- standby: "in standby"
+ alternate_state: "in an unknown state"Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: build
[failure] 1-1:
invalid format: Expected anything but a dictionary for dictionary value @ data['tests'][0]['intent']['slots']['alternate_state']. Got {'on': 'in an unknown state', 'idle': 'not playing', 'standby': 'in standby'}
| @@ -372,6 +382,7 @@ expansion_rules: | |||
| in: "(in|on|at|of)" | |||
| position: "{position}[[ ]%| percent]" | |||
| volume: "{volume:volume_level}[[ ]%| percent]" | |||
| media_players: "([media ]player[s]|speaker[s]stereo[s]|tv[s])" | |||
There was a problem hiding this comment.
Missing a | between speaker and stereo. Should maybe add television? Also, this could be simplified by moving the [s] to the end.
| media_players: "([media ]player[s]|speaker[s]stereo[s]|tv[s])" | |
| media_players: "([media ]player|speaker|stereo|tv|television)[s]" |
There was a problem hiding this comment.
Agreed regarding television, but moving the [s] at the end makes it less readable and does not optimize matching.
An attempt to fix #2229 (at least in English)
These sentences "rephrase" some states that you ask about regarding media players.
Also, when the state is ambiguous (e.g.
is the TV on?), the response clarifies the state, e.g.the TV is on, but it is not playing.As a dependency in this PR, there is a small fix regarding passing a list for the
stateslot, which, although supported in hassil, would have appeared as no matches were found until now.Summary by CodeRabbit
New Features
alternate_stateresponse template for more nuanced state representation.Enhancements
media_playerfrom specific state checks.Tests