-
Notifications
You must be signed in to change notification settings - Fork 2
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 ntp code path #31
Conversation
WalkthroughThe changes involve the removal of 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 (
|
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: 0
Outside diff range and nitpick comments (1)
ovos_PHAL_plugin_system/__init__.py (1)
Line range hint
278-286
: Refactor the service restart logic.The code for restarting the mycroft or ovos service needs to be refactored for better readability, maintainability, and error handling.
Consider the following improvements:
- Handle specific exceptions instead of using a bare
except
block to make it easier to identify the root cause of issues.- Provide feedback to the user if the service restart fails, either through a log message or by emitting a message on the bus.
- Extract the service restart logic into a separate method for better readability and maintainability.
For example:
def restart_mycroft_service(self): try: restart_service(self.core_service_name, sudo=False, user=True) except subprocess.CalledProcessError as e: LOG.warning(f"Failed to restart service as user: {e}") try: restart_service(self.core_service_name, sudo=True, user=False) except subprocess.CalledProcessError as e: LOG.error(f"Failed to restart service as root: {e}") self.bus.emit(Message("system.mycroft.service.restart.failed")) return self.bus.emit(Message("system.mycroft.service.restart.succeeded"))Tools
Ruff
281-281: Do not use bare
except
(E722)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- ovos_PHAL_plugin_system/init.py (3 hunks)
Additional comments not posted (4)
ovos_PHAL_plugin_system/__init__.py (4)
9-10
: LGTM!The import statement is valid and does not introduce any issues.
18-21
: LGTM!The import statements are valid and do not introduce any issues.
312-315
: LGTM!The
SystemEventsAdminValidator
class definition is valid and does not introduce any issues. Thevalidate
method is overridden to always returnTrue
when running as root, and the log message provides useful information for debugging purposes.
Line range hint
319-320
: LGTM!The
SystemEventsAdminPlugin
class definition is valid and does not introduce any issues. The class inherits from bothAdminPlugin
andSystemEventsPlugin
, which provides access to the methods and attributes of both classes. Thevalidator
attribute is set toSystemEventsAdminValidator
, ensuring that the plugin is validated using the appropriatevalidate
method when running as root.
Summary by CodeRabbit
New Features
Bug Fixes
Chores