-
Notifications
You must be signed in to change notification settings - Fork 8
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
STABLE RELEASE - 0.1.0 #264
Conversation
rebase 0.0.38
* rename var for clearity remove extra env var option readme docs add requirements revert env var situation make ovos-config optional add ovos-logs reduce command optics adjustments add `ovos-logs` console script * reorder to account for directories kwarg --------- Co-authored-by: JarbasAi <jarbasai@mailfence.com>
shared constants moved to the common module these enums are used in several places andhaving them elsewhere introduces circular dependencies Co-authored-by: JarbasAi <jarbasai@mailfence.com>
* refactor/ocp_models move the MediaType and Playlist model to utils so they can be reused across ovos modules skills will then be able to return these objects directly in the ocp decorators * refactor/ovos_media * method from OPM * update import * update models * update models --------- Co-authored-by: JarbasAi <jarbasai@mailfence.com>
* OCP serialization improves the serialization and deserialization of the OCP objects makes them compatible with the dicts currently returned by OCP skills, paving the way to allow those skills to return these objects directly in a future workshop release * Update ocp.py --------- Co-authored-by: JarbasAI <33701864+JarbasAl@users.noreply.github.com>
allow initing Playlist object as a regular list Co-authored-by: JarbasAi <jarbasai@mailfence.com>
Updates the requirements on [pexpect](https://github.com/pexpect/pexpect) to permit the latest version. - [Release notes](https://github.com/pexpect/pexpect/releases) - [Changelog](https://github.com/pexpect/pexpect/blob/master/doc/history.rst) - [Commits](pexpect/pexpect@4.6...4.9) --- updated-dependencies: - dependency-name: pexpect dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Updates the requirements on [rapidfuzz](https://github.com/rapidfuzz/RapidFuzz) to permit the latest version. - [Release notes](https://github.com/rapidfuzz/RapidFuzz/releases) - [Changelog](https://github.com/rapidfuzz/RapidFuzz/blob/main/CHANGELOG.rst) - [Commits](rapidfuzz/RapidFuzz@v2.0.0...v3.6.1) --- updated-dependencies: - dependency-name: rapidfuzz dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* fix/detect_ovos_gui_app account for the ovos maintained mycroft-gui fork when checking for running gui processes * add deprecation warning drop bigscreen * add deprecation warning drop bigscreen
* fix/playlist_deserialization * rm debug print
fix update method type check validate that required sei are available ensure metadata is not dropped by the plugin
* Add docstrings and type annotations to `log` module Add test coverage for log rotation Outline test coverage for added log module functions Relates to #2 #239 #233 #250 #253 * Update logs to resolve order-related failure * Add test coverage for `get_logs_config` Refactor variable in `get_logs_config` for clarity Refactor `get_logs_config` for more predictable handling of an empty service name * Add test coverage for `get_log_path` * Add test coverage for `get_log_paths` Update `get_log_paths` to check all log names with `-` normalized to `_` (appeared to be the original intent) Add `enclosure` and `admin` service logs used by Neon and legacy Mycroft/OVOS setups * Add test coverage for `get_available_logs` * Add test coverage for `_monitor_log_level` * Add test of `get_config` call count * Update `get_log_paths` to reference config directly instead of test some well-known values Update unit test for `get_log_paths` * Remove unused `ALL_SERVICES` variable * Remove unused import --------- Co-authored-by: Daniel McKnight <daniel@neon.ai>
WalkthroughThe changes encompass the introduction of new configuration files for dependency management, significant updates to GitHub Actions workflows, and enhancements to various modules within the codebase. Key modifications include the removal of deprecated functions, restructuring of logging and media handling functionalities, and an update to the versioning scheme. Additionally, the documentation has been improved with a new README and a restructured CHANGELOG, while unit tests have been added or modified to align with the latest code changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub
participant Dependabot
participant CI/CD
participant Codebase
User->>GitHub: Push changes
GitHub->>Dependabot: Check for dependency updates
Dependabot->>Codebase: Propose updates
GitHub->>CI/CD: Trigger workflow
CI/CD->>Codebase: Run tests
CI/CD->>GitHub: Report results
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 (
|
fix automation mess up
@coderabbitai review |
Actions performedReview triggered.
|
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: 6
Outside diff range, codebase verification and nitpick comments (15)
requirements/requirements.txt (2)
7-7
: Consider using a more flexible version constraint forcombo-lock
.The exact version
0.2
is specified forcombo-lock
, which may lead to compatibility issues in the future if the library introduces breaking changes.Consider using a more flexible version constraint, such as
~=0.2
, to allow for backward-compatible updates:-combo-lock~=0.2 +combo-lock~=0.2
10-10
: Consider adding a version constraint fororjson
.The absence of a version constraint for
orjson
may lead to compatibility issues in the future if the library introduces breaking changes.Consider adding a version constraint, such as
~=3.8
, to ensure compatibility and allow for backward-compatible updates:-orjson +orjson~=3.8ovos_utils/parse.py (1)
41-41
: Uncomment the debug logging statement.The addition of the debug logging statement is a good change for aiding in debugging. However, the statement is currently commented out.
Uncomment the logging statement to enable logging the selected matching strategy:
-# LOG.debug(f"matching strategy: {strategy}") +LOG.debug(f"matching strategy: {strategy}")README.md (3)
20-22
: Consider adding a comma to improve readability.To enhance the clarity of the sentence, consider adding a comma between the two independent clauses:
- _Different logs can be picked using the `-l` option. All logs will be included if not specified._ - _Optionally the directory where the logs are stored (`-p`) and the file where the slices should be dumped (`-f`) can be specified._ + _Different logs can be picked using the `-l` option. All logs will be included if not specified._, + _Optionally, the directory where the logs are stored (`-p`) and the file where the slices should be dumped (`-f`) can be specified._Tools
LanguageTool
[uncategorized] ~21-~21: A comma might be missing here.
Context: ...ill be included if not specified._ _Optionally the directory where the logs are stored...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
61-62
: Consider adding a comma to improve readability.To enhance the clarity of the sentence, consider adding a comma between the two independent clauses:
- _Different logs can be included using the `-l` option. If not specified, all logs will be included._ - _Optionally the directory where the logs are stored (`-p`) can be specified._ + _Different logs can be included using the `-l` option. If not specified, all logs will be included._, + _Optionally, the directory where the logs are stored (`-p`) can be specified._Tools
LanguageTool
[uncategorized] ~62-~62: Possible missing comma found.
Context: ...ified, all logs will be included._ _Optionally the directory where the logs are stored...(AI_HYDRA_LEO_MISSING_COMMA)
38-38
: Consider updating the horizontal rule style for consistency.To maintain consistent formatting throughout the document, consider updating the horizontal rules at lines 38, 55, and 76 to match the expected style:
-------------- +--------------- --------------------- +--------------- --------------------- +---------------This change is a minor nitpick and does not significantly impact the readability or functionality of the documentation.
Also applies to: 55-55, 76-76
Tools
Markdownlint
38-38: Expected: ---------------; Actual: --------------
Horizontal rule style(MD035, hr-style)
ovos_utils/__init__.py (2)
31-31
: Remove the unnecessary empty line.The empty line at line 31 is unnecessary and can be removed to improve code readability.
Apply this diff to remove the empty line:
-
64-64
: Remove the unnecessary empty line.The empty line at line 64 is unnecessary and can be removed to improve code readability.
Apply this diff to remove the empty line:
-
test/unittests/test_ocp_media.py (1)
1-237
: Great work on the new unit test file! The overall structure and existing test methods look good.Some suggestions for further improvement:
- Implement the test methods marked with TODO comments to ensure comprehensive testing of the
Playlist
class.- Consider adding more test cases to improve coverage, such as edge cases or error handling scenarios.
ovos_utils/system.py (2)
186-204
: LGTM!The addition of the
check_service_installed
function is approved. It adds a useful functionality to the module.Remove the extraneous
f
prefix from the f-string.The f-string at line 196 doesn't contain any placeholders, as indicated by the Ruff static analysis tool. This is a minor issue that can be fixed by removing the extraneous
f
prefix.Apply this diff to fix the minor issue with the f-string:
- installed_base_command = f"systemctl list-unit-files -t service" + installed_base_command = "systemctl list-unit-files -t service"Tools
Ruff
196-196: f-string without any placeholders
Remove extraneous
f
prefix(F541)
289-289
: Remove the unnecessary empty line.The empty line at line 289 is unnecessary and can be removed to improve code readability.
Remove the empty line at line 289.
ovos_utils/log_parser.py (4)
325-325
: Remove extraneousf
prefixes from f-strings without placeholders.The
slice
command uses f-strings for some of the option help texts. However, as pointed out by the static analysis hints, these f-strings do not contain any placeholders, making thef
prefix unnecessary.Remove the
f
prefix from the following lines:
- Line 325:
@click.option("--until", "-u", help=f"end time of the log slice [default: now]")
- Line 327:
@click.option("--paths", "-p", multiple=True, default=get_log_paths(), help=f"the directory logs reside in", show_default=True)
Removing the extraneous
f
prefixes will improve code clarity without affecting functionality.Also applies to: 327-327
Tools
Ruff
325-325: f-string without any placeholders
Remove extraneous
f
prefix(F541)
448-448
: Remove extraneousf
prefixes from f-strings without placeholders.The
list
command uses f-strings for some of the option help texts. However, as pointed out by the static analysis hints, these f-strings do not contain any placeholders, making thef
prefix unnecessary.Remove the
f
prefix from the following lines:
- Line 448:
@click.option("--until", "-u", help=f"end time of the log slice [default: now]")
- Line 450:
@click.option("--paths", "-p", multiple=True, type=click.Path(), default=get_log_paths(), help=f"the directory logs reside in", show_default=True)
Removing the extraneous
f
prefixes will improve code clarity without affecting functionality.Also applies to: 450-450
Tools
Ruff
448-448: f-string without any placeholders
Remove extraneous
f
prefix(F541)
581-581
: Remove extraneousf
prefix from the f-string without placeholders.The
show
command uses an f-string for the option help text. However, as pointed out by the static analysis hint, this f-string does not contain any placeholders, making thef
prefix unnecessary.Remove the
f
prefix from the following line:
- Line 581:
@click.option("--paths", "-p", multiple=True, type=click.Path(), default=get_log_paths(), help=f"the directory logs reside in", show_default=True)
Removing the extraneous
f
prefix will improve code clarity without affecting functionality.Tools
Ruff
581-581: f-string without any placeholders
Remove extraneous
f
prefix(F541)
604-604
: Remove extraneousf
prefix from the f-string without placeholders.The
reduce
command uses an f-string for the option help text. However, as pointed out by the static analysis hint, this f-string does not contain any placeholders, making thef
prefix unnecessary.Remove the
f
prefix from the following line:
- Line 604:
@click.option("--paths", "-p", multiple=True, type=click.Path(), default=get_log_paths(), help=f"the directory logs reside in", show_default=True)
Removing the extraneous
f
prefix will improve code clarity without affecting functionality.Tools
Ruff
604-604: f-string without any placeholders
Remove extraneous
f
prefix(F541)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
test/unittests/test_logs/real.log
is excluded by!**/*.log
Files selected for processing (36)
- .github/dependabot.yml (1 hunks)
- .github/workflows/coverage.yml (1 hunks)
- .github/workflows/publish_alpha.yml (1 hunks)
- .github/workflows/unit_tests.yml (2 hunks)
- CHANGELOG.md (1 hunks)
- README.md (1 hunks)
- ovos_utils/init.py (3 hunks)
- ovos_utils/device_input.py (1 hunks)
- ovos_utils/events.py (4 hunks)
- ovos_utils/file_utils.py (2 hunks)
- ovos_utils/gui.py (4 hunks)
- ovos_utils/json_helper.py (1 hunks)
- ovos_utils/log.py (6 hunks)
- ovos_utils/log_parser.py (1 hunks)
- ovos_utils/messagebus.py (1 hunks)
- ovos_utils/metrics.py (2 hunks)
- ovos_utils/network_utils.py (3 hunks)
- ovos_utils/ocp.py (1 hunks)
- ovos_utils/parse.py (5 hunks)
- ovos_utils/process_utils.py (8 hunks)
- ovos_utils/security.py (1 hunks)
- ovos_utils/signal.py (6 hunks)
- ovos_utils/skills.py (1 hunks)
- ovos_utils/sound.py (1 hunks)
- ovos_utils/ssml.py (2 hunks)
- ovos_utils/system.py (11 hunks)
- ovos_utils/time.py (1 hunks)
- ovos_utils/version.py (1 hunks)
- requirements/extras.txt (1 hunks)
- requirements/requirements.txt (1 hunks)
- setup.py (2 hunks)
- test/unittests/test_events.py (3 hunks)
- test/unittests/test_log.py (3 hunks)
- test/unittests/test_ocp_media.py (1 hunks)
- test/unittests/test_sound.py (2 hunks)
- test/unittests/test_system.py (2 hunks)
Files skipped from review due to trivial changes (13)
- .github/dependabot.yml
- .github/workflows/coverage.yml
- .github/workflows/publish_alpha.yml
- ovos_utils/device_input.py
- ovos_utils/json_helper.py
- ovos_utils/metrics.py
- ovos_utils/network_utils.py
- ovos_utils/process_utils.py
- ovos_utils/security.py
- ovos_utils/ssml.py
- ovos_utils/time.py
- test/unittests/test_sound.py
- test/unittests/test_system.py
Additional context used
LanguageTool
requirements/extras.txt
[locale-violation] ~4-~4: 'workshop' é un xenismo. É preferíbel dicir "taller de traballo"
Context: ...ager>=0.0.25a2 ovos-config>=0.0.12 ovos-workshop>=0.0.13a22 ovos-bus-client >=0.0.8a1(GL_BARBARISM_REPLACE)
[locale-violation] ~5-~5: 'bus' é un xenismo. É preferíbel dicir "faixa-bus"
Context: ...g>=0.0.12 ovos-workshop>=0.0.13a22 ovos-bus-client >=0.0.8a1(GL_BARBARISM_REPLACE)
README.md
[uncategorized] ~21-~21: A comma might be missing here.
Context: ...ill be included if not specified._ _Optionally the directory where the logs are stored...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[grammar] ~25-~25: Did you mean “until” or “up to”?
Context: ...Slice all logs from service start up until now. _[ex: `ovos-logs slice...(UP_UNTIL)
[uncategorized] ~62-~62: Possible missing comma found.
Context: ...ified, all logs will be included._ _Optionally the directory where the logs are stored...(AI_HYDRA_LEO_MISSING_COMMA)
Markdownlint
README.md
38-38: Expected: ---------------; Actual: --------------
Horizontal rule style(MD035, hr-style)
55-55: Expected: ---------------; Actual: ---------------------
Horizontal rule style(MD035, hr-style)
76-76: Expected: ---------------; Actual: ---------------------
Horizontal rule style(MD035, hr-style)
Ruff
ovos_utils/sound.py
107-107: f-string without any placeholders
Remove extraneous
f
prefix(F541)
ovos_utils/system.py
94-94: Redefinition of unused
is_running_from_module
from line 10(F811)
196-196: f-string without any placeholders
Remove extraneous
f
prefix(F541)
ovos_utils/log.py
344-344:
ovos_config.Configuration
imported but unused; consider usingimportlib.util.find_spec
to test for availability(F401)
ovos_utils/ocp.py
596-597: Use a single
if
statement instead of nestedif
statements(SIM102)
ovos_utils/log_parser.py
266-269: Use
return all(log.lower() in get_available_logs(paths) for log in logs)
instead offor
loopReplace with
return all(log.lower() in get_available_logs(paths) for log in logs)
(SIM110)
300-303: Use ternary operator
start = get_last_load_time(directories) if start is None else parse_time(start)
instead ofif
-else
-blockReplace
if
-else
-block withstart = get_last_load_time(directories) if start is None else parse_time(start)
(SIM108)
305-308: Use ternary operator
end = datetime.now() if end is None else parse_time(end)
instead ofif
-else
-blockReplace
if
-else
-block withend = datetime.now() if end is None else parse_time(end)
(SIM108)
325-325: f-string without any placeholders
Remove extraneous
f
prefix(F541)
327-327: f-string without any placeholders
Remove extraneous
f
prefix(F541)
386-386: Local variable
f
is assigned to but never usedRemove assignment to unused variable
f
(F841)
388-388: Do not use bare
except
(E722)
401-401: Use
enumerate()
for index variablelineno
infor
loop(SIM113)
439-439: Use context handler for opening files
(SIM115)
448-448: f-string without any placeholders
Remove extraneous
f
prefix(F541)
450-450: f-string without any placeholders
Remove extraneous
f
prefix(F541)
524-524: Local variable
f
is assigned to but never usedRemove assignment to unused variable
f
(F841)
526-526: Do not use bare
except
(E722)
542-542: Use
enumerate()
for index variablelineno
infor
loop(SIM113)
576-576: Use context handler for opening files
(SIM115)
581-581: f-string without any placeholders
Remove extraneous
f
prefix(F541)
593-593: f-string without any placeholders
Remove extraneous
f
prefix(F541)
597-597: Use context handler for opening files
(SIM115)
604-604: f-string without any placeholders
Remove extraneous
f
prefix(F541)
Additional comments not posted (73)
ovos_utils/messagebus.py (1)
Line range hint
1-3
: LGTM!The code changes are approved. The refactoring has significantly improved the maintainability and clarity of the codebase by removing deprecated functions and classes, and encouraging the use of a new utility module. The import statement is correctly formatted and imports the necessary modules from
ovos_utils.fakebus
.Tools
Ruff
1-1:
ovos_utils.fakebus.dig_for_message
imported but unusedRemove unused import
(F401)
1-1:
ovos_utils.fakebus.FakeMessage
imported but unusedRemove unused import
(F401)
1-1:
ovos_utils.fakebus.Message
imported but unusedRemove unused import
(F401)
1-1:
ovos_utils.fakebus.FakeBus
imported but unusedRemove unused import
(F401)
requirements/extras.txt (5)
5-5
: Verify compatibility with the updatedovos-bus-client
version.Ensure that the code is compatible with
ovos-bus-client
version0.0.8a1
and above, and that there are no breaking changes.Run the following script to search for usages of the
ovos-bus-client
library and verify compatibility:Tools
LanguageTool
[locale-violation] ~5-~5: 'bus' é un xenismo. É preferíbel dicir "faixa-bus"
Context: ...g>=0.0.12 ovos-workshop>=0.0.13a22 ovos-bus-client >=0.0.8a1(GL_BARBARISM_REPLACE)
3-3
: Verify compatibility with the updatedovos-config
version.Ensure that the code is compatible with
ovos-config
version0.0.12
and above, and that there are no breaking changes.Run the following script to search for usages of the
ovos-config
library and verify compatibility:
2-2
: Verify the usage of theovos_plugin_manager
.Ensure that the code is properly integrated with the
ovos_plugin_manager
and follows best practices for plugin development.Run the following script to search for usages of the
ovos_plugin_manager
and verify proper integration:Verification successful
Integration of
ovos_plugin_manager
verified inovos_utils/ocp.py
.The
ovos_plugin_manager
is used for managing stream extractors, with imports foravailable_extractors
andload_stream_extractors
. Note the deprecation notice and TODO comment, which suggest areas for potential cleanup or further review.
- File:
ovos_utils/ocp.py
- Imports:
available_extractors
,load_stream_extractors
fromovos_plugin_manager.ocp
- Deprecation Notice: Transition to using
ovos_plugin_manager
- TODO Comment: Indicates legacy code or incomplete transition
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of the `ovos_plugin_manager` and verify proper integration. # Test: Search for import statements and function calls. # Expect: Proper integration with the plugin system. rg --type python -A 5 $'(import|from) ovos_plugin_manager' rg --type python -A 5 $'ovos_plugin_manager\.'Length of output: 169
Script:
#!/bin/bash # Description: Search for usages of the `ovos_plugin_manager` and verify proper integration. # Test: Search for import statements and function calls. # Expect: Proper integration with the plugin system. rg -A 5 $'(import|from) ovos_plugin_manager' rg -A 5 $'ovos_plugin_manager\.'Length of output: 3165
1-1
: Verify compatibility with the updatedrapidfuzz
version.Ensure that the code is compatible with
rapidfuzz
version3.6
and that there are no breaking changes.Run the following script to search for usages of the
rapidfuzz
library and verify compatibility:
4-4
: Verify the usage of theovos-workshop
.Ensure that the code is properly integrated with the
ovos-workshop
and follows best practices.Run the following script to search for usages of the
ovos-workshop
and verify proper integration:Tools
LanguageTool
[locale-violation] ~4-~4: 'workshop' é un xenismo. É preferíbel dicir "taller de traballo"
Context: ...ager>=0.0.25a2 ovos-config>=0.0.12 ovos-workshop>=0.0.13a22 ovos-bus-client >=0.0.8a1(GL_BARBARISM_REPLACE)
requirements/requirements.txt (3)
1-1
: LGTM!The version update of
pexpect
from4.6
to4.9
is approved. The tilde (~=
) version specifier allows for backward-compatible updates, which is a good practice.
8-8
: LGTM!The addition of
rich-click
at version1.7
is approved. The tilde (~=
) version specifier allows for backward-compatible updates, which is a good practice.
9-9
: LGTM!The addition of
rich
at version13.7
is approved. The tilde (~=
) version specifier allows for backward-compatible updates, which is a good practice.ovos_utils/version.py (2)
4-4
: LGTM!Incrementing the minor version is a common practice when introducing new features or significant changes. The change is consistent with the AI-generated summary.
5-5
: LGTM!Resetting the build version to 0 is a common practice when incrementing the minor version. The change is consistent with the AI-generated summary.
ovos_utils/skills.py (3)
4-26
: LGTM!The
get_non_properties
function is well-implemented and follows good coding practices:
- The function has a clear docstring explaining its purpose and arguments.
- The use of a nested function
check_class
keeps the code modular and readable.- The recursive scan correctly excludes
MycroftSkill
andobject
base classes.- The function returns a set of non-property attributes, which is an appropriate data structure for the result.
The code changes are approved.
29-35
: LGTM!The
skills_loaded
function is well-implemented:
- The function is concise and easy to understand.
- It properly uses the
wait_for_reply
function to wait for a response from the message bus.- The function returns the appropriate status based on the received reply.
- The
bus
argument is optional, allowing flexibility in using a custom message bus if needed.The code changes are approved.
1-1
: LGTM!The import of
wait_for_reply
fromovos_bus_client.util
is relevant and necessary for theskills_loaded
function. The import is placed at the beginning of the file, following the PEP 8 convention.The import is approved.
.github/workflows/unit_tests.yml (1)
40-40
: Dropped support for Python 3.7.The Python version matrix has been updated to remove Python 3.7, leaving only versions 3.8, 3.9, and 3.10. This change indicates a shift towards supporting only more recent versions of Python.
Please ensure that the codebase and its dependencies are compatible with the supported Python versions (3.8, 3.9, and 3.10). Consider running the following script to verify compatibility:
setup.py (4)
54-55
: LGTM!Reading the
README.md
file into thelong_description
variable is a good practice to enhance the package documentation.
71-72
: LGTM!The changes to the
author_email
anddescription
fields are approved.
73-74
: LGTM!Adding the
long_description
andlong_description_content_type
fields is a good practice to improve the package documentation and rendering on package repositories.
75-79
: LGTM!Adding the
entry_points
section to define a console script forovos-logs
is a great way to enhance the usability of the package by providing a command-line interface.ovos_utils/gui.py (5)
1-5
: LGTM!The changes to the import statements improve clarity and reduce dependencies on other modules.
9-12
: LGTM!The changes to the
_default_gui_apps
tuple are consistent with the deprecation of Mycroft components and the shift towards the OVOS ecosystem.
36-42
: LGTM!The changes to the
is_gui_running
function enhance the feedback provided to developers regarding the use of outdated components and are consistent with the deprecation of Mycroft components.
96-96
: LGTM!The change to the log message improves clarity and does not affect the functionality of the code.
Line range hint
1-100
: LGTM!The removal of the
GUIPlaybackStatus
andGUITracker
classes and the deprecation of theextend_about_data
,GUIWidgets
, andGUIInterface
functions and classes are consistent with the refactoring and deprecation of certain classes and methods related to GUI functionality mentioned in the AI-generated summary.The changes suggest a move away from these constructs in favor of a more streamlined approach to GUI management and indicate a potential shift in how GUI interactions are handled in the future. The overall structure of the file has been simplified, with many deprecated methods and classes removed, which should lead to improved maintainability and reduced complexity in the GUI handling code.
The changes reflect a broader transition towards OVOS standards and practices, with an emphasis on deprecating legacy Mycroft components and enhancing the clarity of the codebase.
ovos_utils/signal.py (5)
Line range hint
9-36
: LGTM: The function is correctly marked as deprecated.The deprecation message clearly indicates that the entire
ovos_utils.signal
module is deprecated as of version "0.2.0".Tools
Ruff
25-25: f-string without any placeholders
Remove extraneous
f
prefix(F541)
Line range hint
39-51
: LGTM: The function is correctly marked as deprecated and refactored.
- The deprecation message provides the correct alternative for the function.
- The refactoring simplifies the function by delegating the directory creation to a function imported from
ovos_utils.file_utils
, which reduces code duplication and improves maintainability.
Line range hint
54-66
: LGTM: The function is correctly marked as deprecated.The deprecation message clearly indicates that the entire
ovos_utils.signal
module is deprecated as of version "0.2.0".
Line range hint
69-84
: LGTM: The function is correctly marked as deprecated.The deprecation message clearly indicates that the entire
ovos_utils.signal
module is deprecated as of version "0.2.0".
Line range hint
87-117
: LGTM: The function is correctly marked as deprecated.The deprecation message clearly indicates that the entire
ovos_utils.signal
module is deprecated as of version "0.2.0".ovos_utils/parse.py (3)
22-22
: LGTM!The addition of the new member
DAMERAU_LEVENSHTEIN_SIMILARITY
to theMatchStrategy
enum is approved.
56-57
: LGTM!The addition of the new conditional branch to handle the
DAMERAU_LEVENSHTEIN_SIMILARITY
strategy in thefuzzy_match
function is approved.
Line range hint
1-120
: No additional comments.The rest of the code in the file looks good. No further review is necessary.
README.md (1)
1-87
: Great job on the documentation!The README.md file provides a clear and informative overview of the OVOS-utils project. The installation instructions are straightforward, and the usage examples for the commandline scripts are well-structured and easy to follow. The inclusion of explanations and screenshots enhances the clarity of the examples.
Overall, the documentation changes are well-organized and effectively communicate the purpose and usage of the project.
Tools
LanguageTool
[uncategorized] ~21-~21: A comma might be missing here.
Context: ...ill be included if not specified._ _Optionally the directory where the logs are stored...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[grammar] ~25-~25: Did you mean “until” or “up to”?
Context: ...Slice all logs from service start up until now. _[ex: `ovos-logs slice...(UP_UNTIL)
[uncategorized] ~62-~62: Possible missing comma found.
Context: ...ified, all logs will be included._ _Optionally the directory where the logs are stored...(AI_HYDRA_LEO_MISSING_COMMA)
Markdownlint
38-38: Expected: ---------------; Actual: --------------
Horizontal rule style(MD035, hr-style)
55-55: Expected: ---------------; Actual: ---------------------
Horizontal rule style(MD035, hr-style)
76-76: Expected: ---------------; Actual: ---------------------
Horizontal rule style(MD035, hr-style)
ovos_utils/__init__.py (2)
17-17
: LGTM!The code change is approved.
21-21
: LGTM!The code change is approved.
ovos_utils/sound.py (3)
12-20
: LGTM!The code segment handles the case when
ovos_config
is not available by providing a fallbackread_mycroft_config
function that returns an empty dict. This is a good practice to ensure the code works even when the dependency is missing.
21-26
: LGTM!The code segment creates a custom environment for pulse audio and keeps it separate from the normal environment. This is a good practice to avoid unintended side effects and allow third-party software to choose whether to honor the custom environment.
38-67
: LGTM!The
_find_player
function provides a fallback mechanism to find a suitable player for the given audio file based on the file extension. It checks for various commonly available players and returns the appropriate command to play the file. This is a good approach to handle different audio formats and ensure playback compatibility.CHANGELOG.md (4)
3-5
: LGTM!The addition of the
[Unreleased]
section with a link to theHEAD
of the repository aligns with changelog best practices and provides easy access to the latest changes.
7-23
: Great job organizing the changes!Separating enhancements and bug fixes into distinct sections improves clarity and organization. Including pull request links and authors provides transparency and attribution. The entries are concise and informative.
27-30
: Nice addition of the closed issues section.Including closed issues provides a comprehensive view of the project's progress. The entries are concise and informative.
31-35
: Good job adding the merged pull requests section.Including merged pull requests provides a comprehensive view of the project's progress. The entries are concise and informative.
ovos_utils/system.py (8)
8-8
: LGTM!The import statement is approved.
34-38
: LGTM!The deprecation of the
ntp_sync
function using the@deprecated
decorator is approved.
Line range hint
43-53
: LGTM!The deprecation of the
system_shutdown
function using the@deprecated
decorator is approved.
Line range hint
55-65
: LGTM!The deprecation of the
system_reboot
function using the@deprecated
decorator is approved.
Line range hint
67-73
: LGTM!The deprecation of the
ssh_enable
function using the@deprecated
decorator is approved.
Line range hint
76-83
: LGTM!The deprecation of the
ssh_disable
function using the@deprecated
decorator is approved.
Line range hint
85-92
: LGTM!The deprecation of the
restart_mycroft_service
function using the@deprecated
decorator is approved.
291-291
: LGTM!The comment is approved. It provides useful information about the fallback check using matplotlib.
test/unittests/test_log.py (6)
Line range hint
20-108
: LGTM!The
test_log
method is well-structured and covers various scenarios for the logging functionality. The test cases are comprehensive and cover the important aspects of logging such as log levels, log file paths, log rotation, and diagnostic mode.
109-133
: LGTM!The
test_init_service_logger
method is well-structured and covers the important scenarios for initializing the service logger. The test cases verify that the logger is initialized correctly with the specified configurations and that the config watcher is set up.
187-212
: LGTM!The
test_monitor_log_level
method is well-structured and covers the important scenarios for monitoring log level changes. The test cases verify that the_monitor_log_level
function correctly retrieves the updated log configuration and reinitializes the logger when there are changes.
213-257
: LGTM!The
test_get_logs_config
method is well-structured and covers the important scenarios for retrieving logs configuration. The test cases verify that theget_logs_config
function correctly retrieves the logs configuration based on the provided service name and configuration structure.
274-291
: LGTM!The
test_get_log_paths
method is well-structured and covers the important scenarios for retrieving log paths. The test cases verify that theget_log_paths
function correctly retrieves the log paths based on the provided configuration structure, including default paths and module-specific overrides.
293-305
: LGTM!The
test_get_available_logs
method is well-structured and covers the important scenarios for retrieving available logs. The test cases verify that theget_available_logs
function correctly retrieves the available logs based on the provided log directory structures, including scenarios with logs present and no logs.test/unittests/test_events.py (3)
158-158
: LGTM!The code change is approved.
200-200
: This change is a duplicate of the one at line 188. See the previous comment for the review.
188-188
: Verify the impact of the interface change.The
EventSchedulerInterface
initialization parameter is changed fromname
toskill_id
. Ensure that this change is consistently applied across the codebase and doesn't break any existing functionality.Run the following script to verify the usage of the changed parameter:
Verification successful
Change from
name
toskill_id
is correctly applied and isolated to the test file.The
EventSchedulerInterface
initialization parameter change fromname
toskill_id
is only present in thetest/unittests/test_events.py
file. There are no other instances of this interface being instantiated elsewhere in the codebase, indicating that the change is isolated and does not affect other parts of the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the changed `EventSchedulerInterface` initialization parameter. # Test: Search for the instantiation of `EventSchedulerInterface`. Expect: Only occurrences of the new `skill_id` parameter. rg --type python -A 5 $'EventSchedulerInterface\('Length of output: 82
Script:
#!/bin/bash # Description: Verify the usage of the changed `EventSchedulerInterface` initialization parameter. # Test: Search for the instantiation of `EventSchedulerInterface`. Expect: Only occurrences of the new `skill_id` parameter. rg --type py -A 5 $'EventSchedulerInterface\('Length of output: 1141
Script:
#!/bin/bash # Description: Verify the usage of the changed `EventSchedulerInterface` initialization parameter across the entire codebase. # Test: Search for the instantiation of `EventSchedulerInterface` in the entire codebase. Expect: Only occurrences of the new `skill_id` parameter. rg -A 5 $'EventSchedulerInterface\('Length of output: 1131
ovos_utils/file_utils.py (2)
19-46
: LGTM!The new
ensure_directory_exists
function is well-implemented and follows best practices:
- It properly joins the directory and domain using
os.path.join
.- It normalizes and expands the directory path.
- It sets the appropriate permissions using
os.umask
.- It handles errors and logs warnings.
The code changes are approved.
90-90
: LGTM!The changes to the
get_cache_directory
function are appropriate:
- The comment update clarifies that only Linux uses RAM for the cache directory.
- The conditional check change is more specific and consistent with the updated comment, indicating that the RAM-based cache directory functionality is now limited to Linux systems only.
The code changes are approved.
Also applies to: 95-95
ovos_utils/log.py (5)
15-15
: LGTM!The new import statements are valid and necessary for the added functionality.
Also applies to: 21-23
83-97
: LGTM!The changes in the
init
method simplify the logic and make it more robust. Retrieving the log level from the config and setting it using theset_level
method is a good practice.
185-251
: LGTM!The new functions
_monitor_log_level
,init_service_logger
, andget_logs_config
enhance the logging capabilities of the module, making it more adaptable to different service configurations. Theget_logs_config
function checks for both service-specific and general logging configurations, which is a good practice.
320-398
: LGTM!The new functions
get_log_path
,get_log_paths
, andget_available_logs
provide a straightforward way to retrieve log file names and directories based on service-specific configurations or default paths. The functions handle cases where theovos_config
module is not available, which is a good practice.Tools
Ruff
344-344:
ovos_config.Configuration
imported but unused; consider usingimportlib.util.find_spec
to test for availability(F401)
344-344
: Skipping the hint from Ruff.The hint is a false positive because the
Configuration
class is used in theget_log_path
function. The suggestion to useimportlib.util.find_spec
is not applicable in this case.Tools
Ruff
344-344:
ovos_config.Configuration
imported but unused; consider usingimportlib.util.find_spec
to test for availability(F401)
ovos_utils/events.py (2)
209-210
: LGTM!The changes to the constructor of the
EventSchedulerInterface
class are approved. Removing the deprecatedname
andsched_id
parameters and defaultingskill_id
to the lowercase class name streamlines the initialization process and improves clarity.
224-231
: LGTM!The changes to the
set_id
method of theEventSchedulerInterface
class are approved. Replacing thesched_id
parameter withskill_id
reinforces the consistent usage ofskill_id
throughout the class.ovos_utils/ocp.py (5)
133-145
: LGTM!The code changes are approved.
148-154
: LGTM!The code changes are approved.
157-253
: LGTM!The code changes are approved.
256-336
: LGTM!The code changes are approved.
338-599
: LGTM!The code changes are approved.
Tools
Ruff
596-597: Use a single
if
statement instead of nestedif
statements(SIM102)
def is_running_from_module(module_name): | ||
# Stack: | ||
# [0] - _log() | ||
# [1] - debug(), info(), warning(), or error() | ||
# [2] - caller | ||
stack = inspect.stack() | ||
|
||
# Record: | ||
# [0] - frame object | ||
# [1] - filename | ||
# [2] - line number | ||
# [3] - function | ||
# ... | ||
for record in stack[2:]: | ||
mod = inspect.getmodule(record[0]) | ||
name = mod.__name__ if mod else '' | ||
# module name in file path of caller | ||
# or import name matches module name | ||
if f"/{module_name}/" in record[1] or \ | ||
name.startswith(module_name.replace("-", "_").replace(" ", "_")): | ||
return True | ||
return False |
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.
Remove the duplicate function definition.
The is_running_from_module
function defined at line range 94-115 is an exact duplicate of the function defined at line 10. This violates the DRY (Don't Repeat Yourself) principle.
Remove the duplicate function definition at line range 94-115.
Tools
Ruff
94-94: Redefinition of unused
is_running_from_module
from line 10(F811)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
ovos-utils
library.Chores