fix:finish extracting intent classes from adapt package #248
Conversation
ports usage of "excludes" needed for OpenVoiceOS/ovos-core#527 companion to OpenVoiceOS/ovos-adapt-pipeline-plugin#6
WalkthroughThe pull request introduces substantial modifications to the Changes
Poem
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: 1
🧹 Outside diff range and nitpick comments (3)
ovos_workshop/intents.py (3)
Line range hint
26-39: Update the docstring of theIntentconstructor to include theexcludesparameterThe
__init__method of theIntentclass now includes theexcludesparameter, but the docstring does not document this parameter. Please update the docstring to include a description ofexcludesand its purpose.
52-65: Reflect exclusion logic in thevalidate_with_tagsmethod docstringThe
validate_with_tagsmethod now includes logic to handle excluded entities. The docstring should be updated to mention that it checks for excluded entities before validating required and optional entities, and that it returns a confidence of 0.0 if any excluded entities are found.
Line range hint
559-585: Handle 'excludes' in themunge_intent_parserfunctionThe
munge_intent_parserfunction currently modifies therequires,optional, andat_least_oneattributes to make them skill-exclusive. With the addition of theexcludesattribute to theIntentclass, please update the function to also munge theexcludeslist to ensure consistency and correct behavior.Apply this diff to handle
excludesinmunge_intent_parser:def munge_intent_parser(intent_parser, name, skill_id): # Existing code... # Munge optional keywords opts = [] for i in intent_parser.optional: if not i[0].startswith(skill_id): kw = (skill_id + i[0], skill_id + i[0]) opts.append(kw) else: opts.append(i) intent_parser.optional = opts + # Munge excludes keywords + excludes = [] + for entity in intent_parser.excludes: + if not entity.startswith(skill_id): + excludes.append(skill_id + entity) + else: + excludes.append(entity) + intent_parser.excludes = excludes # Munge at_least_one keywords at_least_one = [] for i in intent_parser.at_least_one: element = [skill_id + e.replace(skill_id, '') for e in i] at_least_one.append(tuple(element)) intent_parser.at_least_one = at_least_one
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #248 +/- ##
==========================================
- Coverage 53.41% 49.58% -3.84%
==========================================
Files 37 37
Lines 4362 4439 +77
==========================================
- Hits 2330 2201 -129
- Misses 2032 2238 +206 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
test/unittests/skills/test_mycroft_skill/test_mycroft_skill.py (1)
Line range hint
112-267: Consider enhancing tests for 'excludes' functionalityWhile the addition of the 'excludes' field to the expected results is correct and consistent across all relevant test methods, consider enhancing the tests to more thoroughly validate the 'excludes' functionality:
- Add test cases that explicitly check the behavior of intents with non-empty 'excludes' lists.
- Include tests that verify the runtime behavior of intents with exclusions, not just their registration.
- Consider adding edge cases, such as circular exclusions or exclusions of non-existent intents.
These additions would provide more comprehensive coverage of the new functionality and help catch potential edge cases or unexpected behaviors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- test/unittests/skills/test_mycroft_skill/test_mycroft_skill.py (5 hunks)
🧰 Additional context used
🔇 Additional comments (5)
test/unittests/skills/test_mycroft_skill/test_mycroft_skill.py (5)
112-113: LGTM: 'excludes' field added to expected intent registration result.The addition of the 'excludes' field to the expected result in the
test_register_intentmethod is consistent with the changes described in the PR objectives. This update correctly tests for the new functionality of excluding intents.
125-126: LGTM: 'excludes' field added to expected intent registration result for IntentBuilder.The addition of the 'excludes' field to the expected result for the IntentBuilder object in the
test_register_intentmethod is correct and consistent with the previous change. This ensures that the test covers the new functionality for both Intent and IntentBuilder objects.
147-148: LGTM: 'excludes' field added to expected result in enable/disable intent test.The addition of the 'excludes' field to the expected result in the
test_enable_disable_intentmethod is appropriate. This change ensures that the test correctly verifies the presence of the 'excludes' field when enabling an intent.
167-168: LGTM: 'excludes' field added to expected result in enable/disable intent handlers test.The addition of the 'excludes' field to the expected result in the
test_enable_disable_intent_handlersmethod is correct. This change ensures that the test verifies the presence of the 'excludes' field when enabling an intent through the intent handlers.
266-267: LGTM: 'excludes' field added to expected result in register decorators test.The addition of the 'excludes' field to the expected result in the
test_register_decoratorsmethod is appropriate. This change ensures that the test verifies the presence of the 'excludes' field when using decorated intents.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
ovos_workshop/intents.py (2)
Line range hint
26-39: Add unit tests for the new 'excludes' functionalityThe
Intentclass now includes anexcludesparameter to handle excluded entities. To ensure this new feature works correctly and to prevent future regressions, it's important to add unit tests that cover scenarios involving excluded entities.Would you like assistance in creating unit tests for the
excludesparameter?
272-283: Improve the docstring formatting for theexcludemethodThe docstring for the
excludemethod lacks proper formatting and clarity.Apply this diff to enhance readability and conform to documentation standards:
""" - The intent parser must not contain an entity of the provided type. - - Args: - entity_type(str): an entity type - - Returns: - self: to continue modifications. - """ + """The intent parser must not contain an entity of the provided type. + + Args: + entity_type (str): The entity type to exclude. + + Returns: + IntentBuilder: The instance of IntentBuilder to allow method chaining. + """This clarifies the purpose of the method and correctly formats the docstring.
| CLIENT_ENTITY_NAME = 'Client' # TODO - ??? what is this magic string | ||
|
|
||
| target_client, canonical_form, confidence = \ | ||
| self._find_first_tag(local_tags, CLIENT_ENTITY_NAME) |
There was a problem hiding this comment.
Clarify the purpose of CLIENT_ENTITY_NAME and address the TODO comment
The constant CLIENT_ENTITY_NAME is set to 'Client', accompanied by a TODO comment questioning its purpose:
CLIENT_ENTITY_NAME = 'Client' # TODO - ??? what is this magic stringIt's important to address this TODO comment to improve code clarity. If CLIENT_ENTITY_NAME is necessary, please provide documentation explaining its role. If it's unused or obsolete, consider removing it to clean up the codebase.
Would you like assistance in determining the necessity of CLIENT_ENTITY_NAME and updating the code accordingly?
ports usage of "excludes"
needed for OpenVoiceOS/ovos-core#527
companion to OpenVoiceOS/ovos-adapt-pipeline-plugin#6
Summary by CodeRabbit
excludesparameter in theIntentclass to specify entities that should not be included.IntentBuilderclass to support the exclusion of specific entities during intent construction.excludesfunctionality, ensuring accurate intent management.