deprecate!:backend client #604
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request involves the deletion of a GitHub Actions workflow file that defined a testing process for project constraints. Additionally, it introduces significant modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 #604 +/- ##
==========================================
- Coverage 75.33% 73.13% -2.21%
==========================================
Files 15 15
Lines 3094 1526 -1568
==========================================
- Hits 2331 1116 -1215
+ Misses 763 410 -353
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
github-actions
bot
added
breaking
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
mycroft/skills/settings.py (5)
175-179: Consider removing redundant pairing check inuploadmethod.The
uploadmethod is decorated with@requires_backend, which should handle backend availability and device pairing. The additional checkif not is_paired():may be redundant. Removing this check can simplify the code and reduce duplication.
185-189: Consider removing redundant pairing check inupload_metamethod.Similar to the
uploadmethod,upload_metahas a redundantif not is_paired():check while already being decorated with@requires_backend. Simplifying the code by removing this check can improve maintainability.
195-199: Consider removing redundant pairing check indownloadmethod.The
downloadmethod's check foris_paired()may be unnecessary due to the@requires_backenddecorator. Eliminating redundant checks helps streamline the code.
229-254: Extendsettings2metato handle additional data types like floats.Currently,
settings2metahandlesbool,str, andinttypes but does not account forfloatvalues. To ensure all numerical settings are properly included in the metadata, consider adding support forfloattypes.Apply the following changes to handle
floattypes:for k, v in settings.items(): if k.startswith("_"): continue label = k.replace("-", " ").replace("_", " ").title() if isinstance(v, bool): # existing code... elif isinstance(v, str): # existing code... - elif isinstance(v, int): + elif isinstance(v, int) or isinstance(v, float): fields.append({ "name": k, "type": "number", "label": label, "value": str(v) }) else: # Optionally handle other data types or raise an error
153-154: Offer assistance to implement support for settingsmeta XDG paths.There's a TODO comment indicating the need to support settingsmeta XDG paths. Proper implementation of XDG paths can enhance compatibility with different system configurations. If you need assistance with this feature, I'd be happy to help.
Do you want me to generate a solution or open a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
mycroft/skills/settings.py(1 hunks)requirements/extra-deprecated.txt(1 hunks)requirements/requirements.txt(0 hunks)
💤 Files with no reviewable changes (1)
- requirements/requirements.txt
🧰 Additional context used
🪛 Ruff
mycroft/skills/settings.py
72-72: mycroft.deprecated.skills.settings.SkillSettingsDownloader imported but unused
Remove unused import
(F401)
72-72: mycroft.deprecated.skills.settings.SettingsMetaUploader imported but unused
Remove unused import
(F401)
72-72: mycroft.deprecated.skills.settings.load_remote_settings_cache imported but unused
Remove unused import
(F401)
73-73: mycroft.deprecated.skills.settings.save_remote_settings_cache imported but unused
Remove unused import
(F401)
73-73: mycroft.deprecated.skills.settings.REMOTE_CACHE imported but unused
Remove unused import
(F401)
🔇 Additional comments (2)
requirements/extra-deprecated.txt (2)
12-12: Verify impact of deprecating ovos-backend-client
The addition of ovos-backend-client to deprecated requirements aligns with the PR objective. However, we should verify that all dependent modules are prepared for this deprecation.
#!/bin/bash
# Description: Check for modules that might be affected by the backend client deprecation
# Search for imports or usage of ovos-backend-client
rg -l "from ovos.backend|from ovos_backend|import ovos.backend|import ovos_backend"
# Search for potential configuration files that might reference the backend
fd -e yaml -e json | xargs rg -l "backend.*client|ovos.*backend"Line range hint 15-16: Review dependency conflict with ovos-listener
The existing note indicates a dependency conflict that blocks ovos-bus-client 1.0.0. Since we're making dependency changes, this might be a good time to address this conflict.
Consider:
- Evaluating if the ovos-listener conflict needs to be resolved alongside this deprecation
- Documenting any known limitations or issues that users might face due to these dependency constraints
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
test/end2end/session/test_get_response.py (2)
Line range hint
431-431: Use a more pythonic way to check for empty listsIn the
test_nestedmethod, the conditionif not len(items):can be simplified toif not items:, which is more pythonic and enhances readability.Apply this diff to simplify the condition:
- if not len(items): + if not items:
Line range hint
709-710: Avoid directly modifying the context of received messagesIn the
abort_responsefunction withintest_kill_response_with_session_mismatch, you are directly modifyingmsg.context["session"]. Modifying the context of incoming messages can lead to unexpected side effects. Instead, create a newMessagewith the updated context.Apply this diff to safely create a new message:
- msg.context["session"] = Session("456").serialize() - self.core.bus.emit(msg.forward("mycroft.skills.abort_question", - {"skill_id": self.skill_id})) + new_context = dict(msg.context) + new_context["session"] = Session("456").serialize() + new_msg = Message("mycroft.skills.abort_question", + {"skill_id": self.skill_id}, + new_context) + self.core.bus.emit(new_msg)
github-actions
bot
added
breaking
github-actions
bot
added
breaking
companion to OpenVoiceOS/OVOS-workshop#295
Summary by CodeRabbit
New Features
SkillSettingsManagerfor improved skill settings synchronization with backend services.Chores
ovos-backend-clientin the deprecated requirements.