[DE] Add intent for removing items from shopping list #2397
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @Lesekater
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
|
Started draft for core repo: |
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughA new intent, Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
sentences/de/shopping_list_HassShoppingListRemoveItem.yaml (2)
3-10: LGTM: Intent definition and sentences are well-structured.The
HassShoppingListRemoveItemintent is well-defined with a comprehensive set of sentence structures for removing items from a shopping list in German. The use of optional verb endings (e.g., "entfern[e]") is a good practice for handling both formal and informal forms of address.Consider adding a sentence structure that uses the accusative case, such as:
- "(entfern[e]|streich[e]|nehm[e]|nimm) <item> (von|aus) <meine_liste_akkusativ>"This would provide more flexibility in sentence construction.
11-14: LGTM: Expansion rules are well-defined, but accusative case is unused.The expansion rules for list names in both dative and accusative cases are well-defined, providing flexibility in referring to the shopping list. The item placeholder correctly references a shopping list item.
The accusative case expansion rule (
meine_liste_akkusativ) is defined but not used in the current sentence structures. To make use of this rule, consider adding a sentence structure that uses the accusative case, as suggested in the previous comment. Alternatively, if the accusative case is not needed, you could remove the unused expansion rule to keep the file concise.tests/de/shopping_list_HassShoppingListRemoveItem.yaml (2)
19-22: Intent definition is correct, but there's an extra space in the slot value.The intent name "HassShoppingListRemoveItem" accurately reflects the action of removing an item from a shopping list. The slot "item" is correctly defined.
However, there's an extra space after "milch" in the slot value. This might cause issues in intent matching or item processing.
Consider removing the extra space:
- item: "milch " + item: "milch"
23-23: Response is correct but could be more generic.The response "milch entfernt" correctly confirms the removal of the item in German. However, it's specifically tailored to "milch" (milk) and might not be suitable for other items.
Consider making the response more generic to accommodate any item. For example:
response: "{item} entfernt"This way, the response will dynamically include the name of the removed item.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- intents.yaml (1 hunks)
- responses/de/HassShoppingListRemoveItem.yaml (1 hunks)
- sentences/de/shopping_list_HassShoppingListRemoveItem.yaml (1 hunks)
- tests/de/shopping_list_HassShoppingListRemoveItem.yaml (1 hunks)
🔇 Additional comments not posted (6)
sentences/de/shopping_list_HassShoppingListRemoveItem.yaml (2)
1-2: LGTM: File structure and language declaration are correct.The file correctly declares German (de) as the language and follows the expected YAML structure for intent definitions.
1-14: Overall, excellent implementation with minor suggestions for improvement.This YAML file successfully defines the
HassShoppingListRemoveItemintent for German language users, providing a comprehensive set of sentence structures for removing items from a shopping list. The implementation is well-structured and follows best practices for intent definition.To further improve the file:
- Consider adding a sentence structure that uses the accusative case to make use of the
meine_liste_akkusativexpansion rule.- If the accusative case is not needed, consider removing the unused expansion rule to keep the file concise.
These minor adjustments would enhance the consistency and efficiency of the intent definition.
tests/de/shopping_list_HassShoppingListRemoveItem.yaml (1)
1-1: LGTM: Language specification is correct.The language is correctly specified as "de" for German, which matches the content of the test sentences.
intents.yaml (3)
265-273: LGTM: New intent structure is well-defined and consistent.The new
HassShoppingListRemoveItemintent is well-structured and consistent with other intents in the file. It correctly defines the necessary properties and a required slot for removing an item from the shopping list.
265-273: Verify intent purpose: Removing vs. Adding itemsThe PR objectives mention adding an intent for adding items to shopping lists, but this new intent is for removing items. Please clarify if this is intentional or if an additional intent for adding items should be included.
265-273: Good: Consistent with existing shopping list intent and completes the set of operationsThe new
HassShoppingListRemoveItemintent is consistent with the existingHassShoppingListAddItemintent. Together, they provide a complete set of operations for managing shopping list items (adding and removing), which enhances the functionality of the shopping list feature.
|
First, many thanks for your contribution 👍
I think we should first have the core PR merged, before adding intents, therefore I'm setting this PR here to draft mode, as long as the core PR is not merged. Furthermore it might also need an discussion first 🤔 |
Lesekater
changed the title
Co-authored-by: Michael <[email protected]>
Co-authored-by: Michael <[email protected]>
|
FYI @Lesekater @mib1185 The linked core PR has been closed. ../Frenck |
Thx for mentioning it. Yes, i closed it, after working on a similar PR for the todo component, where allenporter mentioned, that it would probably be better to focus on the mark item as completed intent rather than the more destructive remove item itent (home-assistant/core#127046 (comment)). I opened another core PR for marking items as complete on the shopping list (home-assistant/core#128565). I could probably create a new PR here too if the core PR goes through, but I think most of the wording can stay the same between item completion and item removal. |
|
I would vote for closing this one here, since it was designed and targeted to another approach. |
Added a intent to add items to shopping lists.
Summary by CodeRabbit
New Features
Tests