fix:log_spam #566
fix:log_spam #566
Conversation
WalkthroughThe changes in 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #566 +/- ##
==========================================
- Coverage 75.33% 75.20% -0.14%
==========================================
Files 15 15
Lines 3094 1553 -1541
==========================================
- Hits 2331 1168 -1163
+ Misses 763 385 -378
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ 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: 0
🧹 Outside diff range and nitpick comments (5)
ovos_core/intent_services/__init__.py (5)
94-96: LGTM! Consider updating documentation.The change from
@deprecateddecorator tolog_deprecationis a good approach. It provides more flexibility in handling deprecations and informs users about future changes.Consider updating the class or module documentation to reflect this deprecation and the planned transition to plugins.
100-101: LGTM! Consider refactoring for DRY principle.The consistent application of
log_deprecationacross multiple properties is good. It ensures a uniform approach to deprecation warnings.However, the repetitive nature of these changes presents an opportunity for refactoring. Consider creating a helper method or decorator to apply this deprecation logging, which would adhere to the DRY (Don't Repeat Yourself) principle and make future updates easier.
Also applies to: 106-107, 112-114, 119-120, 125-126, 131-132, 137-138
459-462: LGTM! Consider creating a tracking issue.The addition of the try-except block for
setup_localeimproves the robustness of the code by handling potential exceptions. The comment about uncoupling lingua franca from the core indicates a planned architectural change.Consider creating a tracking issue or adding a TODO comment with a ticket number for the future task of making skills responsible for locale loading. This will ensure the planned change is not forgotten and can be properly prioritized.
🧰 Tools
🪛 Ruff
461-461: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
Line range hint
1-750: Overall improvements with opportunities for further refinement.The changes in this file generally improve the codebase by:
- Providing more informative deprecation warnings.
- Ensuring proper resource cleanup during shutdown.
- Adding robustness to locale handling.
However, there are opportunities for further improvement:
- Refactor the repetitive deprecation logging in properties to adhere to the DRY principle.
- Create tracking issues for planned architectural changes (e.g., uncoupling lingua franca from the core).
- Verify the removal of event handler deregistrations to prevent potential issues.
Consider addressing these points in future updates to enhance code maintainability and prevent potential bugs.
Event Handler Deregistrations Not Fully Removed
The verification script found that several
self.bus.removecalls are still present inovos_core/intent_services/__init__.pyand associated test files. Please ensure that these event handler deregistrations are either no longer needed or have been handled appropriately elsewhere in the codebase to prevent potential memory leaks or unexpected behavior.
self.bus.remove('register_vocab', self.handle_register_vocab)self.bus.remove('register_intent', self.handle_register_intent)self.bus.remove('recognizer_loop:utterance', self.handle_utterance)self.bus.remove('detach_intent', self.handle_detach_intent)self.bus.remove('detach_skill', self.handle_detach_skill)self.bus.remove('add_context', self.handle_add_context)self.bus.remove('remove_context', self.handle_remove_context)self.bus.remove('clear_context', self.handle_clear_context)self.bus.remove('mycroft.skills.loaded', self.update_skill_name_dict)self.bus.remove('intent.service.intent.get', self.handle_get_intent)self.bus.remove('intent.service.skills.get', self.handle_get_skills)self.bus.remove('intent.service.adapt.get', self.handle_get_adapt)self.bus.remove('intent.service.adapt.manifest.get', self.handle_adapt_manifest)self.bus.remove('intent.service.adapt.vocab.manifest.get', self.handle_vocab_manifest)self.bus.remove('intent.service.padatious.get', self.handle_get_padatious)self.bus.remove('intent.service.padatious.manifest.get', self.handle_padatious_manifest)self.bus.remove('intent.service.padatious.entities.manifest.get', self.handle_entity_manifest)🔗 Analysis chain
Line range hint
723-750: Verify removal of event handler deregistrations.The addition of shutdown calls for
utterance_pluginsandmetadata_pluginsis good for proper resource cleanup.However, several
self.bus.removecalls for event handlers have been removed. Please verify that these event handlers are indeed no longer needed or are handled elsewhere in the codebase to prevent potential memory leaks or unexpected behavior.To verify the removal of event handlers, run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the removed event handlers # Test: Search for references to removed event handlers rg --type python "self\.bus\.(on|remove)\s*\(\s*'(register_vocab|register_intent|recognizer_loop:utterance|detach_intent|detach_skill|add_context|remove_context|clear_context|mycroft\.skills\.loaded|intent\.service\.intent\.get|intent\.service\.skills\.get|intent\.service\.adapt\.get|intent\.service\.adapt\.manifest\.get|intent\.service\.adapt\.vocab\.manifest\.get|intent\.service\.padatious\.get|intent\.service\.padatious\.manifest\.get|intent\.service\.padatious\.entities\.manifest\.get)'"Length of output: 535
Script:
#!/bin/bash # Description: List all supported file types in ripgrep to identify the correct type for Python files rg --type-listLength of output: 4821
Script:
#!/bin/bash # Description: Check for any remaining references to the removed event handlers using the correct file type identifier rg --type py "self\.bus\.(on|remove)\s*\(\s*'(register_vocab|register_intent|recognizer_loop:utterance|detach_intent|detach_skill|add_context|remove_context|clear_context|mycroft\.skills\.loaded|intent\.service\.intent\.get|intent\.service\.skills\.get|intent\.service\.adapt\.get|intent\.service\.adapt\.manifest\.get|intent\.service\.adapt\.vocab\.manifest\.get|intent\.service\.padatious\.get|intent\.service\.padatious\.manifest\.get|intent\.service\.padatious\.entities\.manifest\.get)'"Length of output: 4603
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- ovos_core/intent_services/init.py (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
ovos_core/intent_services/__init__.py (2)
20-23: LGTM! New import may affect intent handling.The import changes look good. The addition of
open_intent_envelopefromovos_workshop.intentssuggests a potential enhancement or refactoring in intent handling. Ensure that this new functionality is properly documented and tested.
143-144: Consistent with getters. Refactoring opportunity remains.The setter methods have been updated consistently with their corresponding getters, which is good for maintaining symmetry in the deprecation approach.
As mentioned for the getter methods, consider refactoring these deprecation warnings into a single helper method or decorator to reduce code duplication and improve maintainability.
Also applies to: 149-150, 155-156, 161-162, 167-168, 173-174, 179-180, 185-186
|
All good. |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation