-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor!:deprecate QML upload from bus #120
base: dev
Are you sure you want to change the base?
Conversation
WalkthroughThe changes involve significant modifications to the Changes
Sequence Diagram(s)(No sequence diagrams generated as the changes do not warrant a visual representation of control flow.) 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 (
|
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
marked as breaking because it does change the public api, but nothing in our org is using the removed stuff so it should be safe it may also cause issues in existing docker installs, but GUI in docker never worked quite right, the goal is precisely to make docker work correctly, if all containers are updated this won't cause issues there either |
never worked right, causes more issues than it helps
never worked right, causes more issues than it helps
346ff66
to
fa11187
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- ovos_bus_client/apis/gui.py (7 hunks)
🧰 Additional context used
🔇 Additional comments (5)
ovos_bus_client/apis/gui.py (5)
105-106
: Initialization now includes caching GUI filesThe
__init__
method now calls_cache_gui_files()
, which caches GUI resources upon initialization. Ensure that this change does not introduce unintended side effects during object creation.Including GUI caching in initialization streamlines resource management. However, please ensure that any exceptions raised during caching are properly handled to prevent initialization failures.
316-316
: RaisingValueError
for file paths in_normalize_page_name
The
_normalize_page_name
method now raises aValueError
if a file path is provided instead of a resource name. This enforces proper usage of the method.This change improves input validation and prevents potential misuse by ensuring that only resource names (not file paths) are accepted.
319-321
: Logging error for deprecated.qml
extension usageThe method logs an error when a page name includes the
.qml
extension, guiding developers to omit the extension.By logging this error, developers are alerted to update their code, aligning with the deprecation of including extensions in resource names.
328-328
: Addition ofremove_others
parameter inshow_page
methodThe
show_page
method now includes aremove_others
parameter with a default value ofFalse
. This parameter allows control over whether to remove other pages when showing a new one.The addition enhances functionality by providing flexibility in how pages are displayed. Ensure that this parameter is clearly documented for developers.
83-86
: Removal ofremote_server
parameter may introduce breaking changesThe
remote_server
parameter has been removed from the__init__
method of theGUIInterface
class. This alteration changes the public API and can lead to issues if external code relies on this parameter.Please verify that no external code or downstream projects are instantiating
GUIInterface
with theremote_server
parameter. Run the following script to identify any such instances:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
ovos_bus_client/apis/gui.py (1)
Line range hint
1-651
: Summary of changes and suggestionsThe changes in this file focus on three main areas:
- Removing the remote server functionality
- Adding a new caching mechanism for GUI files
- Deprecating the use of ".qml" extensions in page names
These changes simplify the
GUIInterface
class and prepare it for future updates. However, there are a few areas that could be improved:
- Add error handling to the new
_cache_gui_files
method to make it more robust.- Update the class and method docstrings to reflect the changes, especially the removal of the
remote_server
parameter and the deprecation of ".qml" extensions.- Ensure that all calls to
show_pages
andremove_pages
within the codebase are updated to exclude the ".qml" extension.Consider creating a migration guide or deprecation timeline for the removal of ".qml" extension support, as this change might affect existing skills or plugins that rely on the current behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- ovos_bus_client/apis/gui.py (14 hunks)
🧰 Additional context used
🔇 Additional comments (2)
ovos_bus_client/apis/gui.py (2)
316-321
: Improved input validation in_normalize_page_name
The changes to the
_normalize_page_name
method enhance input validation by raising aValueError
for file paths and provide more informative error messages. This improvement helps prevent potential security issues and guides developers to use the correct resource naming convention.
404-410
: Deprecation warning for.qml
extension inremove_pages
methodSimilar to
show_pages
, theremove_pages
method now warns if page names include the.qml
extension and normalizes them. This change maintains consistency with theshow_pages
method and prepares for future updates.Ensure consistency across the codebase by updating any calls to
remove_pages
to exclude the.qml
extension, aligning with the updated usage guidelines.
never worked right, causes more issues than it helps
companion to OpenVoiceOS/ovos-gui#53
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
GUIInterface
class by eliminating deprecated functionalities, reducing complexity.