refactor:upstream classes #6
Conversation
some code in the wild still imports from adapt directly
WalkthroughThe changes primarily involve the Changes
Possibly related PRs
Suggested labels
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- ovos_adapt/intent.py (6 hunks)
🧰 Additional context used
🔇 Additional comments (6)
ovos_adapt/intent.py (6)
18-18: Necessary imports added for metaclasses and type annotationsThe addition of
abc,Optional, andListimports is appropriate to support the use of metaclasses and type annotations in the updated classes.Also applies to: 20-20
121-130: Implementation of_IntentMetaensures backward compatibilityThe introduction of the
_IntentMetametaclass with the overridden__instancecheck__method effectively maintains backward compatibility with the originalIntentclass fromadapt.intent. This approach is sound and correctly implemented.
132-134: UpdatedIntentclass with metaclass and type hintsUpdating the
Intentclass to use_IntentMetaas its metaclass ensures consistent behavior with the newly introduced compatibility layer. The addition of type hints in the constructor enhances code readability and type safety.
144-146: Initialization avoids mutable default argumentsAssigning attributes with
self.requires = requires or []and similar statements prevents issues associated with mutable default arguments in function definitions. This is a best practice in Python and is properly applied here.
237-246: Implementation of_IntentBuilderMetamaintains backward compatibilityThe addition of the
_IntentBuilderMetametaclass with the overridden__instancecheck__method ensures backward compatibility with the originalIntentBuilderclass fromadapt.intent. This is appropriately implemented.
248-248: UpdatedIntentBuilderclass to use the new metaclassBy specifying
metaclass=_IntentBuilderMeta, theIntentBuilderclass aligns with the compatibility adjustments made for theIntentclass, ensuring consistent behavior across both classes.
use the OVOS workshop Intent and IntentBuilder classes , as these will be shared with other keyword based engines
JarbasAl
changed the title
ports usage of "excludes" needed for OpenVoiceOS/ovos-core#527 companion to OpenVoiceOS/ovos-adapt-pipeline-plugin#6
ports usage of "excludes" needed for OpenVoiceOS/ovos-core#527 companion to OpenVoiceOS/ovos-adapt-pipeline-plugin#6
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- ovos_adapt/intent.py (4 hunks)
- requirements.txt (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- requirements.txt
🧰 Additional context used
🪛 Ruff
ovos_adapt/intent.py
18-18:
ovos_workshop.intent.IntentBuilderimported but unusedRemove unused import:
ovos_workshop.intent.IntentBuilder(F401)
* fix:finish extracting intent classes from adapt package ports usage of "excludes" needed for OpenVoiceOS/ovos-core#527 companion to OpenVoiceOS/ovos-adapt-pipeline-plugin#6 * fix test * simplify * add a test
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
ovos_adapt/intent.py (1)
Line range hint
1-87: Overall refactoring improves centralization but introduces potential redundancyThe changes in this file primarily involve refactoring to delegate functionality to static methods of the
Intentclass. This centralization can improve maintainability and consistency of the codebase. However, it also introduces wrapper functions that might be unnecessary if all callers can directly use theIntentclass methods.Consider the following recommendations:
- Evaluate the necessity of the wrapper functions (
find_first_tagandresolve_one_of). If they're not needed for backward compatibility or API simplicity, consider removing them to reduce code complexity.- If you decide to keep the wrapper functions, document the reason for their existence to prevent future confusion.
- Ensure that the
Intentclass and its static methods are well-documented and easily discoverable for developers who might need to use them directly.These changes appear to be part of a larger refactoring effort. Ensure that all related changes across the codebase are consistent with this new architecture.
🧰 Tools
🪛 Ruff
19-19:
ovos_workshop.intents.IntentBuilderimported but unusedRemove unused import:
ovos_workshop.intents.IntentBuilder(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- ovos_adapt/intent.py (3 hunks)
- requirements.txt (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 Additional context used
🪛 Ruff
ovos_adapt/intent.py
19-19:
ovos_workshop.intents.IntentBuilderimported but unusedRemove unused import:
ovos_workshop.intents.IntentBuilder(F401)
needs OpenVoiceOS/OVOS-workshop#248