deprecate!:backend client #295
Conversation
WalkthroughThe changes involve significant modifications to 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #295 +/- ##
==========================================
- Coverage 53.41% 51.57% -1.84%
==========================================
Files 37 36 -1
Lines 4362 4178 -184
==========================================
- Hits 2330 2155 -175
+ Misses 2032 2023 -9 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![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)
test/unittests/test_abstract_app.py (3)
Line range hint
12-14: Fix critical typo in__init__method nameThere's a typo in the method name
__int__which should be__init__. This prevents proper initialization of the Application class and could lead to unexpected behavior in tests.Apply this fix:
- def __int__(self, *args, **kwargs): + def __init__(self, *args, **kwargs):
Line range hint
67-68: Address TODO comments in test methodsThese test methods are marked as TODO but appear to be testing important functionality:
test_get_language_dir: Language directory handlingtest_clear_intents: Intent managementHaving these tests implemented would improve test coverage and ensure proper functionality.
Would you like me to help implement these test methods or create GitHub issues to track them?
Also applies to: 71-72
Line range hint
29-46: Improve robustness of settings path test cleanupWhile the test includes cleanup of temporary files, it could be more robust by using a try-finally block to ensure cleanup even if assertions fail.
Consider refactoring the test like this:
def test_settings_path(self): self.assertIn("/apps/", self.app.settings_path) # Test settings path conflicts test_app = OVOSAbstractApplication(skill_id="test", bus=self.bus) test_skill = OVOSSkill(skill_id="test", bus=self.bus) - - # Test app vs skill base directories - self.assertIn("/apps/", test_app.settings_path) - self.assertIn("/skills/", test_skill.settings_path) - - # Test settings changes - test_skill.settings['is_skill'] = True - test_app.settings['is_skill'] = False - self.assertTrue(test_skill.settings['is_skill']) - self.assertFalse(test_app.settings['is_skill']) - - # Cleanup test files - remove(test_app.settings_path) - remove(test_skill.settings_path) + try: + # Test app vs skill base directories + self.assertIn("/apps/", test_app.settings_path) + self.assertIn("/skills/", test_skill.settings_path) + + # Test settings changes + test_skill.settings['is_skill'] = True + test_app.settings['is_skill'] = False + self.assertTrue(test_skill.settings['is_skill']) + self.assertFalse(test_app.settings['is_skill']) + finally: + # Cleanup test files + for path in [test_app.settings_path, test_skill.settings_path]: + try: + remove(path) + except FileNotFoundError: + passovos_workshop/app.py (2)
17-24: Update docstring to reflect parameter changesThe docstring needs to be updated to remove references to the removed parameters.
Apply this diff to update the documentation:
""" Create an Application. An application is essentially a skill, but designed such that it may be run without an intent service. @param skill_id: Unique ID for this application @param bus: MessageBusClient to bind to application @param resources_dir: optional root resource directory (else defaults to application `root_dir` + @param gui: GUIInterface to bind (if `None`, one is created) + @param kwargs: Additional keyword arguments passed to OVOSSkill """
Line range hint
16-35: Architectural changes align with backend client deprecationThe removal of settings management from the constructor aligns with the broader goal of deprecating the backend client. The changes simplify the application initialization while maintaining core functionality.
Consider documenting these architectural changes in the project's documentation to help users migrate their applications.
Would you like me to help create migration documentation or examples for updating existing applications?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
ovos_workshop/app.py(1 hunks)test/unittests/skills/test_base.py(0 hunks)test/unittests/test_abstract_app.py(1 hunks)
💤 Files with no reviewable changes (1)
- test/unittests/skills/test_base.py
🔇 Additional comments (2)
test/unittests/test_abstract_app.py (1)
24-24: LGTM: Simplified application initialization
The removal of settings-related parameters aligns with the broader initiative to simplify settings management across the codebase.
ovos_workshop/app.py (1)
16-16:
Breaking change: Parameters removed from constructor
The removal of lang, settings, and enable_settings_manager parameters is a breaking change that could affect existing applications.
Let's verify the impact on existing applications:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
OVOSSkillclass by removing unnecessary components and parameters.ovos-backend-client, indicating a shift in project requirements.Tests
TestSettingsclass to clean up the test suite.