Change wake word id to string #960
Conversation
WalkthroughThe changes involve significant modifications to the data types within the Protocol Buffers schema for voice assistant functionality. Specifically, fields related to wake words in several messages have transitioned from numeric types to string types. Additionally, new message types have been introduced to enhance the API's capabilities in managing various entities and their states. Changes
Possibly related PRs
Suggested labels
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #960 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 17 17
Lines 2671 2671
=========================================
Hits 2671 2671 ☔ View full report in Codecov by Sentry. |
![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 and nitpick comments (1)
aioesphomeapi/api_pb2.py (1)
Line range hint
1-28164: Significant API update - looks good overall, but client impact should be consideredThis is a substantial update to the API that introduces many new message types and expands existing ones. The changes enable the API to handle a wider variety of entities and support new features, which is great to see.
From a design perspective, the new message types and enums follow consistent naming conventions and structure, keeping the API clean and comprehensible. The overall architecture of the API looks sound and well-thought-out.
However, given the extent of these changes, it's important to consider the impact on existing client implementations. Any clients using an older version of the API will likely need to be updated to account for the new message types and fields.
I recommend:
- Clearly communicating these API changes to existing users, along with a migration guide if needed.
- Considering a versioning strategy for the API if one is not already in place, so that future updates can be made without breaking existing clients.
Other than that, the API changes LGTM! Let me know if you have any other specific areas you'd like me to review in more detail.
Tools
Ruff
17-17: Module level import not at top of file
(E402)
17-17:
.api_options_pb2imported but unusedRemove unused import:
.api_options_pb2(F401)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- aioesphomeapi/api.proto (3 hunks)
- aioesphomeapi/api_pb2.py (1 hunks)
- aioesphomeapi/client.py (1 hunks)
- aioesphomeapi/model.py (2 hunks)
- tests/test_client.py (2 hunks)
- tests/test_model.py (2 hunks)
Additional context used
Path-based instructions (6)
tests/test_model.py (1)
Pattern
**: - Do not generate or add any sequence diagramsaioesphomeapi/model.py (1)
Pattern
**: - Do not generate or add any sequence diagramsaioesphomeapi/api.proto (1)
Pattern
**: - Do not generate or add any sequence diagramsaioesphomeapi/client.py (1)
Pattern
**: - Do not generate or add any sequence diagramstests/test_client.py (1)
Pattern
**: - Do not generate or add any sequence diagramsaioesphomeapi/api_pb2.py (1)
Pattern
**: - Do not generate or add any sequence diagrams
Additional comments not posted (8)
tests/test_model.py (1)
699-699: Approve the test case update to reflect theactive_wake_wordstype change.The test case has been updated to use a string value (
"1234") for theactive_wake_wordsattribute, which aligns with the change in theVoiceAssistantConfigurationResponsemodel to expect a list of strings instead of integers.This change suggests a shift in how wake word identifiers are represented, likely to support more flexible formats beyond just integers. It's a significant change that could have implications throughout the codebase.
To ensure the robustness of this change, verify that all code paths interacting with the
VoiceAssistantConfigurationResponsemodel have been updated to handle the new string type foractive_wake_words. Run the following script to check for any potential mismatches:If any occurrences are found where
active_wake_wordsis still being treated as integers, update those instances to handle strings instead.Also applies to: 710-710
Verification successful
Confirm consistent implementation of
active_wake_wordstype changeThe verification process has confirmed that the change from integer to string values for
active_wake_wordshas been consistently implemented across the test files. All occurrences found in the codebase use the new string format ["1234"]:
- tests/test_model.py:710
- tests/test_client.py:2694
- tests/test_client.py:2712
No instances of integer values or inconsistent usage were detected. The change appears to be properly reflected in both test_model.py and test_client.py.
While no issues were found in the current codebase, it's worth noting that we didn't encounter any production code using
active_wake_words. If production code exists in a separate repository, a manual review of that code is recommended to ensure full consistency of this change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for occurrences of active_wake_words being assigned or compared to integer values rg --type python 'active_wake_words\s*=\s*\[\s*\d+\s*\]' rg --type python 'active_wake_words\s*==\s*\[\s*\d+\s*\]' # Check for occurrences of active_wake_words being assigned or compared to string values rg --type python 'active_wake_words\s*=\s*\[\s*[\'\"].*[\'\"]\s*\]' rg --type python 'active_wake_words\s*==\s*\[\s*[\'\"].*[\'\"]\s*\]'Length of output: 274
Script:
#!/bin/bash # Find Python files and search for active_wake_words assignments or comparisons fd -e py | xargs rg 'active_wake_words\s*=\s*\[' -n fd -e py | xargs rg 'active_wake_words\s*==\s*\[' -n # Search for string assignments or comparisons fd -e py | xargs rg 'active_wake_words\s*=\s*\[.*"\]' -n fd -e py | xargs rg 'active_wake_words\s*==\s*\[.*"\]' -nLength of output: 693
aioesphomeapi/api.proto (3)
1619-1619: Verify the usage of theactive_wake_wordsfield in the codebase.The change from
repeated uint32torepeated stringfor theactive_wake_wordsfield in theVoiceAssistantSetConfigurationmessage aligns with the changes in the related messages and maintains consistency in the data types used for wake word identifiers.Please ensure that all usages of the
active_wake_wordsfield in the codebase are updated to handle the newrepeated stringtype.Run the following script to verify the usage of the
active_wake_wordsfield:Verification successful
Verification successful:
active_wake_wordsfield correctly updatedThe
active_wake_wordsfield in theVoiceAssistantSetConfigurationmessage has been successfully updated torepeated stringtype. This change is consistent across all occurrences in the./aioesphomeapi/api.protofile. No other usages were found in other.protofiles, indicating that the change is localized and correctly implemented.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of the `active_wake_words` field in `VoiceAssistantSetConfiguration` are updated to handle the `repeated string` type. # Test: Search for the field usage. Expect: Only occurrences with the `repeated string` type. rg --type proto $'VoiceAssistantSetConfiguration' -A 5Length of output: 83
Script:
#!/bin/bash # Description: Search for VoiceAssistantSetConfiguration message and active_wake_words field in .proto files # Find all .proto files and search for VoiceAssistantSetConfiguration echo "Searching for VoiceAssistantSetConfiguration message:" fd -e proto -x grep -Hn "message VoiceAssistantSetConfiguration" {} + echo -e "\nSearching for active_wake_words field:" fd -e proto -x grep -Hn "active_wake_words" {} +Length of output: 757
1610-1610: Verify the usage of theactive_wake_wordsfield in the codebase.The change from
repeated uint32torepeated stringfor theactive_wake_wordsfield in theVoiceAssistantConfigurationResponsemessage aligns with the change in theVoiceAssistantWakeWordmessage and maintains consistency in the data types used for wake word identifiers.Please ensure that all usages of the
active_wake_wordsfield in the codebase are updated to handle the newrepeated stringtype.Run the following script to verify the usage of the
active_wake_wordsfield:Verification successful
Change verified:
active_wake_wordsfield updated correctlyThe
active_wake_wordsfield in theVoiceAssistantConfigurationResponsemessage (./aioesphomeapi/api.proto:1604) has been successfully updated torepeated string active_wake_words = 2;. This change aligns with the modification mentioned in the original review comment and maintains consistency with theVoiceAssistantWakeWordmessage update.No other occurrences of
active_wake_wordswere found in the .proto files, suggesting this is the only place where it's defined. The presence ofuint32 max_active_wake_words = 3;indicates that the number of active wake words is still limited, which is consistent with the previous implementation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of the `active_wake_words` field in `VoiceAssistantConfigurationResponse` are updated to handle the `repeated string` type. # Test: Search for the field usage. Expect: Only occurrences with the `repeated string` type. rg --type proto $'VoiceAssistantConfigurationResponse' -A 5Length of output: 88
Script:
#!/bin/bash # Description: Search for VoiceAssistantConfigurationResponse and active_wake_words in .proto files # Find .proto files and search for VoiceAssistantConfigurationResponse echo "Searching for VoiceAssistantConfigurationResponse:" fd -e proto -x grep -Hn "message VoiceAssistantConfigurationResponse" {} \; # Find .proto files and search for active_wake_words within VoiceAssistantConfigurationResponse echo -e "\nSearching for active_wake_words field:" fd -e proto -x awk '/message VoiceAssistantConfigurationResponse/,/^}/' {} | grep "active_wake_words"Length of output: 542
1593-1593: Verify the usage of theidfield in the codebase.The change from
uint32tostringfor theidfield in theVoiceAssistantWakeWordmessage is consistent with the PR objective and allows for more flexible identifiers.Please ensure that all usages of the
idfield in the codebase are updated to handle the newstringtype and that the identifiers are unique and consistent.Run the following script to verify the usage of the
idfield:Verification successful
Change from
uint32tostringforidfield inVoiceAssistantWakeWordis correctly implemented and compatible with existing usage.The modification of the
idfield fromuint32tostringin theVoiceAssistantWakeWordmessage has been successfully implemented in theaioesphomeapi/api.protofile. The generated Python code inapi_pb2.pyreflects this change, and existing usages in test files are already compatible with the new string type. No issues or inconsistencies were found in the codebase related to this change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of the `id` field in `VoiceAssistantWakeWord` are updated to handle the `string` type. # Test: Search for the field usage. Expect: Only occurrences with the `string` type. rg --type proto $'VoiceAssistantWakeWord' -A 5Length of output: 75
Script:
#!/bin/bash # Description: Find all .proto files and search for VoiceAssistantWakeWord and id usage # Find all .proto files proto_files=$(fd -e proto) # Search for VoiceAssistantWakeWord and id in .proto files for file in $proto_files; do echo "File: $file" rg -C 5 'VoiceAssistantWakeWord|id' "$file" echo "---" doneLength of output: 39167
Script:
#!/bin/bash # Description: Search for VoiceAssistantWakeWord usage in all files # Search for VoiceAssistantWakeWord in all files echo "Searching for VoiceAssistantWakeWord usage:" rg -C 5 'VoiceAssistantWakeWord' # Search for specific usage of id field with VoiceAssistantWakeWord echo "Searching for specific usage of id field with VoiceAssistantWakeWord:" rg -C 5 'VoiceAssistantWakeWord.*id'Length of output: 109138
aioesphomeapi/client.py (1)
1477-1479: Verify that all callers have been updated to passlist[str].The
active_wake_wordsparameter type change fromlist[int]tolist[str]looks good as it allows for more flexibility in wake word identifiers.However, please ensure that all code paths invoking
set_voice_assistant_configurationhave been updated to pass a list of strings instead of integers to avoid any runtime type errors.Run the following command to find all usages of
set_voice_assistant_configurationand verify the argument types:Verification successful
Changes have been correctly implemented and tested.
The verification process has confirmed that the
set_voice_assistant_configurationfunction is correctly using thelist[str]type foractive_wake_words. Additionally, the test casetest_set_voice_assistant_configurationdemonstrates the correct usage by passing a list of strings["1234"].Key findings:
- The main implementation accepts
active_wake_words: list[str]as expected.- The test case calls the function with a list of strings, confirming that callers have been updated.
These results indicate that the type change has been properly implemented and tested, addressing the concerns raised in the initial review comment.
Scripts executed
The following scripts were executed for the analysis:
Script:
rg --type python -A 5 -B 5 $'set_voice_assistant_configuration'Length of output: 93
Script:
#!/bin/bash # Find all Python files and search for 'set_voice_assistant_configuration' fd -e py -x rg -A 5 -B 5 'set_voice_assistant_configuration' {}Length of output: 1194
tests/test_client.py (2)
2712-2716: Test looks good!The
test_set_voice_assistant_configurationtest correctly verifies that theset_voice_assistant_configurationmethod sends the expectedVoiceAssistantSetConfigurationmessage with the providedactive_wake_wordsvalue.
2689-2694: Test looks good!The
test_get_voice_assistant_configurationtest correctly verifies the behavior of theget_voice_assistant_configurationmethod. It ensures that the method sends the expectedVoiceAssistantConfigurationRequestmessage and correctly parses and returns the receivedVoiceAssistantConfigurationResponsemessage as aVoiceAssistantConfigurationResponseModelinstance.aioesphomeapi/api_pb2.py (1)
Line range hint
20-24: LGTMThe descriptor pool initialization looks good. The serialized file is being added correctly.
Tools
Ruff
17-17: Module level import not at top of file
(E402)
17-17:
.api_options_pb2imported but unusedRemove unused import:
.api_options_pb2(F401)
| @@ -1300,7 +1300,7 @@ class VoiceAssistantAnnounceFinished(APIModelBase): | |||
|
|
|||
| @_frozen_dataclass_decorator | |||
| class VoiceAssistantWakeWord(APIModelBase): | |||
| id: int | |||
| id: str | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Update required in test file, minimal impact elsewhere.
The change of the id attribute in the VoiceAssistantWakeWord class from int to str has been implemented correctly in the class definition. However, one test file needs to be updated:
- In
tests/test_model.py, update theidvalue from1to"1":VoiceAssistantWakeWord( id="1", # Change this line wake_word="okay nabu", trained_languages=["en"], )
The impact of this change appears to be minimal:
- The
from_dictandfrom_pbmethods are generic and should handle theidas a string without modifications. - No other direct usages of
VoiceAssistantWakeWord.idwere found in the codebase. - The test in
tests/test_client.pyalready uses a string for theidand doesn't need to be changed.
Analysis chain
Breaking change: id attribute changed from int to str.
The id attribute in the VoiceAssistantWakeWord class has been changed from int to str. This is a breaking change.
Verify that all usages of the id attribute across the codebase are updated to handle it as a string. Pay special attention to:
- Serialization and deserialization of
VoiceAssistantWakeWordobjects. - Comparisons or arithmetic operations involving the
idattribute.
Run the following script to find usages of the id attribute:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of the `id` attribute of `VoiceAssistantWakeWord` class.
# Test: Search for accesses to the `id` attribute. Expect: Only usages treating `id` as a string.
rg --type python -A 5 $'VoiceAssistantWakeWord\\.(id|from_dict|from_pb)'
Length of output: 103
Script:
#!/bin/bash
# Description: Find Python files and search for VoiceAssistantWakeWord class usage
# Find all Python files
echo "Python files in the repository:"
fd '\.py$'
echo "\nSearching for VoiceAssistantWakeWord usage:"
# Search for VoiceAssistantWakeWord in Python files
fd '\.py$' -x rg -n 'VoiceAssistantWakeWord' {}
echo "\nSearching for VoiceAssistantWakeWord.id usage:"
# Search for VoiceAssistantWakeWord.id usage
fd '\.py$' -x rg -n 'VoiceAssistantWakeWord.*\.id' {}
echo "\nSearching for VoiceAssistantWakeWord instantiation:"
# Search for VoiceAssistantWakeWord instantiation
fd '\.py$' -x rg -n 'VoiceAssistantWakeWord\(' {}
Length of output: 52504
Script:
#!/bin/bash
# Description: Examine VoiceAssistantWakeWord class definition and related methods
echo "VoiceAssistantWakeWord class definition:"
rg -A 20 "class VoiceAssistantWakeWord" aioesphomeapi/model.py
echo "\nfrom_dict method:"
rg "def from_dict" aioesphomeapi/model.py -A 10
echo "\nfrom_pb method:"
rg "def from_pb" aioesphomeapi/model.py -A 10
echo "\nTest file instantiations:"
rg "VoiceAssistantWakeWord\(" tests/ -C 5
Length of output: 3564
| @@ -1320,7 +1320,7 @@ | |||
| available_wake_words: list[VoiceAssistantWakeWord] = converter_field( | |||
| default_factory=list, converter=VoiceAssistantWakeWord.convert_list | |||
| ) | |||
| active_wake_words: list[int] = converter_field(default_factory=list, converter=list) | |||
| active_wake_words: list[str] = converter_field(default_factory=list, converter=list) | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Inconsistency found in VoiceAssistantSetConfiguration class
The active_wake_words attribute has been successfully changed from list[int] to list[str] in most parts of the codebase. However, there's an inconsistency in the VoiceAssistantSetConfiguration class in aioesphomeapi/model.py:
class VoiceAssistantSetConfiguration(APIModelBase):
active_wake_words: list[int] = converter_field(default_factory=list, converter=list)This class still defines active_wake_words as list[int], which is inconsistent with the rest of the implementation. To maintain consistency and prevent potential issues, this should be updated to list[str].
Action items:
- Update the
VoiceAssistantSetConfigurationclass inaioesphomeapi/model.pyto uselist[str]foractive_wake_words. - After making this change, thoroughly test the voice assistant configuration functionality to ensure no regressions occur.
Analysis chain
Breaking change: active_wake_words attribute changed from list[int] to list[str].
The active_wake_words attribute in the VoiceAssistantConfigurationResponse class has been changed from list[int] to list[str]. This is a breaking change.
Verify that all usages of the active_wake_words attribute across the codebase are updated to handle it as a list of strings. Pay special attention to:
- Serialization and deserialization of
VoiceAssistantConfigurationResponseobjects. - Comparisons or iterations involving the
active_wake_wordsattribute.
Run the following script to find usages of the active_wake_words attribute:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of the `active_wake_words` attribute of `VoiceAssistantConfigurationResponse` class.
# Test: Search for accesses to the `active_wake_words` attribute. Expect: Only usages treating `active_wake_words` as a list of strings.
rg --type python -A 5 $'VoiceAssistantConfigurationResponse\\.(active_wake_words|from_dict|from_pb)'
Length of output: 131
Script:
#!/bin/bash
# Description: Search for occurrences of 'active_wake_words' and 'VoiceAssistantConfigurationResponse' in the codebase
# Search for 'active_wake_words'
echo "Searching for 'active_wake_words':"
rg -n "active_wake_words" -A 3 -B 1
echo -e "\n----------------------------------------\n"
# Search for 'VoiceAssistantConfigurationResponse'
echo "Searching for 'VoiceAssistantConfigurationResponse':"
rg -n "VoiceAssistantConfigurationResponse" -A 3 -B 1
Length of output: 109951
Script:
#!/bin/bash
# Search for any occurrences of 'active_wake_words' that might still be using integers
echo "Searching for 'active_wake_words' with potential integer usage:"
rg -n "active_wake_words.*int" -A 3 -B 1
echo -e "\n----------------------------------------\n"
# Search for code that processes 'active_wake_words'
echo "Searching for code that processes 'active_wake_words':"
rg -n "active_wake_words" -A 5 -B 1 | grep -v "ListEntities" | grep -v "Response" | grep -v "Request"
Length of output: 5179
Change
idfield ofVoiceAssistantWakeWordfromuint32tostring.