[EN] HassGetState optimizations #2224
Conversation
|
Warning Rate limit exceeded@frenck has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe recent updates to the Home Assistant Changes
Sequence Diagram(s) (Beta)No sequence diagrams are necessary for these changes, as they primarily involve updates to sentence structures and responses without altering control flows or introducing new features. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I have a branch where I'm looking to condense some of these common patterns in I can rebase my proposal on this regardless and submit that elsewhere, but I thought I'd at least float it since you're making a bigger change to this file anyway. |
|
There are other |
frenck
force-pushed
the
en-light-getstate
branch
from
d40f394 to
fdb2010
Compare
| - (is|are)[ there] any {on_off_domains:domain} {on_off_states:state} | ||
| - (is|are)[ there] any {on_off_domains:domain} ({on_off_states:state};in <area>) | ||
| - (is|are)[ there] any <area> {on_off_domains:domain} {on_off_states:state} |
There was a problem hiding this comment.
| - (is|are)[ there] any {on_off_domains:domain} {on_off_states:state} | |
| - (is|are)[ there] any {on_off_domains:domain} ({on_off_states:state};in <area>) | |
| - (is|are)[ there] any <area> {on_off_domains:domain} {on_off_states:state} | |
| - <are_any> {on_off_domains:domain} {on_off_states:state} [<in_area_floor>] | |
| - <are_any> {on_off_domains:domain} <in_area_floor> {on_off_states:state} | |
| - <are_any> <area> {on_off_domains:domain} {on_off_states:state} | |
| - <is> <area> {on_off_domains:domain} on |
Use the new <are_any> and <is> expansions as well as <in_area_floor>. Additionally, I think it's easier to parse the optional area suffix on the first sentence than the (foo;bar) syntax, so that's split among the two lines.
Final suggestion is to add an is area domain on? here and leave is area domain off? in the all sentences.
Not sure if there is a way for us to only reference on from on_off_states:state. It may be useful to do that because we could add [(switched|turned|set [to])] on into the common on_off_states.
| - (do you know|tell me) if there are any {on_off_domains:domain} {on_off_states:state} | ||
| - (do you know|tell me) if there are any {on_off_domains:domain} ({on_off_states:state};in <area>) | ||
| - (do you know|tell me) if there are any <area> {on_off_domains:domain} {on_off_states:state} |
There was a problem hiding this comment.
I think it might be easier to read if we add [(do you know|tell me) if there] as an optional prefix in the above sentences.
| - are [all ][[of ]the ]{on_off_domains:domain} [(switched|turned|set[ to]) ]{on_off_states:state} | ||
| - are [all ][of ]<area> {on_off_domains:domain} [(switched|turned|set[ to]) ]{on_off_states:state} | ||
| - are [all ][[of ]the ]{on_off_domains:domain} ([(switched|turned|set[ to]) ]{on_off_states:state};in <area>) |
There was a problem hiding this comment.
Use <are_all> to match up to are all of the. I also again think it is easier to read if we split out the (foo;bar) expansion again. Also, adding the area off query here.
| - are [all ][[of ]the ]{on_off_domains:domain} [(switched|turned|set[ to]) ]{on_off_states:state} | |
| - are [all ][of ]<area> {on_off_domains:domain} [(switched|turned|set[ to]) ]{on_off_states:state} | |
| - are [all ][[of ]the ]{on_off_domains:domain} ([(switched|turned|set[ to]) ]{on_off_states:state};in <area>) | |
| - <are_all> {on_off_domains:domain} [(switched|turned|set [to])] {on_off_states:state} [<in_area_floor>] | |
| - <are_all> <area_floor> {on_off_domains:domain} [(switched|turned|set [to])] {on_off_states:state} | |
| - <are_all> {on_off_domains:domain} ([(switched|turned|set [to])] <in_area_floor> {on_off_states:state} | |
| - <is> <area> {on_off_domains:domain} off |
| - sentences: | ||
| - are all [the] {on_off_domains:domain} [(switched|turned|set[ to])] {on_off_states:state} [in <area>] | ||
| - are all [the] {on_off_domains:domain} in <area> [(switched|turned|set[ to])] {on_off_states:state} | ||
| - is the light [(switched|turned|set[ to]) ]{on_off_states:state} | ||
| - is the light ([(switched|turned|set[ to]) ]{on_off_states:state};in <area>) | ||
| - is <area> light [(switched|turned|set[ to]) ]{on_off_states:state} | ||
| slots: | ||
| domain: light | ||
| response: all | ||
|
|
There was a problem hiding this comment.
This could then be deleted
| - "[do you know ](which|what) {on_off_domains:domain} (is|are) {on_off_states:state}" | ||
| - "[do you know ](which|what) {on_off_domains:domain} ((is|are) {on_off_states:state};in <area>)" | ||
| - "[do you know ](which|what) <area> {on_off_domains:domain} (is|are) {on_off_states:state}" |
There was a problem hiding this comment.
Maybe do you know should be added to the skip words. They seem redundant in any kind of query sentence. Same with tell me as we have below.
You can also replace (which|what) with <which> and (is|are) with <is>. I'd also suggest a similar removal of the (foo;bar) as above.
| - "[tell me ]how many {on_off_domains:domain} (is|are) {on_off_states:state}" | ||
| - "[tell me ]how many {on_off_domains:domain} ((is|are) {on_off_states:state};in <area>)" | ||
| - "[tell me ]how many <area> {on_off_domains:domain} (is|are) {on_off_states:state}" |
There was a problem hiding this comment.
As above, consider moving tell me to skip words. Replace how many with <how_many> and (is|are) with <is>, and break up the (foo;bar).
| response: how_many | ||
|
|
||
| - sentences: | ||
| - (do you know|tell me|<what_is>) [the [current ](state|value) of ]<name>[ in <area>] |
There was a problem hiding this comment.
Probably use <in_area_floor>. Also, [current] (state|value) should be replaced by <state>, but will need you to add value to the state expansion.
| - is [the ][state of ]<name> {on_off_states:state} | ||
| - is [the ][state of ]<name> (({on_off_states:state};in <area>)) |
There was a problem hiding this comment.
is [the] can be replaced by <is>. Also remove the permutation as shown above.
| response: one_yesno | ||
| excludes_context: | ||
| domain: | ||
| - cover |
There was a problem hiding this comment.
Why is this excluded? Because there is a collision with a different response there?
| - sentences: | ||
| - "are all the lights on in the kitchen" | ||
| - "are all lights turned on in the kitchen?" | ||
| - "are all lights in the kitchen on?" | ||
| - "is the kitchen light on?" |
There was a problem hiding this comment.
If we make the suggested changes, this should move to an "any" response and not all.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Apart from optimizing the sentences for
HassGetStatea bit, I've also introduced a new set of sentences specifically for querying lights.Is the light off in the kitchenusually means asking if all of the lights are off.Inspired by #2218
Summary by CodeRabbit
New Features
Refactor
Tests