deprecate:is_classic_core #242
Conversation
WalkthroughThe changes primarily 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 (
|
![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_skill.py (2)
98-100: LGTM: Test logic simplifiedThe removal of the
is_classic_core()check simplifies the test logic and aligns with the PR objective. The unconditional assertion forCoreSkillis now appropriate given the deprecation of classic core support.Consider updating the comment to reflect the current state more accurately:
# The metaclass ensures this always returns True in OVOS # as we no longer support running under mycroft-coreThis minor change would clarify that support for mycroft-core has been completely removed, rather than implying a lack of control over it.
137-144: LGTM: OVOS-specific event checks addedThe addition of OVOS-specific event checks and the removal of the
is_classic_core()conditional (as mentioned in the AI summary) align well with the PR objective. This change enhances test coverage for OVOS-specific functionality.Consider adding a brief comment above this block to explain the purpose of these new checks:
# Check for OVOS-specific events that are now always expected default_ovos = [...]This would provide context for future developers about why these specific events are being checked.
ovos_workshop/skills/ovos.py (3)
Line range hint
334-339: Typographical error in log message invoc_match_cachesetterThere's a missing space in the log message, causing "Thisfunctionality" to be concatenated without a space:
self.log.warning("self._voc_cache should not be modified externally. This" "functionality will be deprecated in a future release")Consider adding a space at the beginning of the second string or combining the strings for clarity.
Apply this diff to correct the log message:
-self.log.warning("self._voc_cache should not be modified externally. This" - "functionality will be deprecated in a future release") +self.log.warning("self._voc_cache should not be modified externally. " + "This functionality will be deprecated in a future release")
Line range hint
352-354: Typographical error in deprecation message fortranslate_namedvaluesThe deprecation message for
translate_namedvaluescontains a typo:@deprecated("Use `self.resources.load_named_value_filetext, data) instead", "0.1.0")It should be corrected to:
@deprecated("Use `self.resources.load_named_value_file`", "0.1.0")This ensures that developers are directed to the correct replacement method.
Apply this diff to fix the deprecation message:
-@deprecated("Use `self.resources.load_named_value_filetext, data) instead", "0.1.0") +@deprecated("Use `self.resources.load_named_value_file`", "0.1.0")
Line range hint
397-403: Suggestion: Move import statement outside of__new__method inOVOSFallbackSkillCurrently, the import for
FallbackSkilloccurs within the__new__method:def __new__(cls, *args, **kwargs): log_deprecation("Implement " "`ovos_workshop.skills.fallback.FallbackSkill`", "0.1.0") from ovos_workshop.skills.fallback import FallbackSkill return FallbackSkill(*args, **kwargs)It's recommended to place imports at the top of the file to improve readability and avoid redundant imports if the method is called multiple times.
Apply this diff to move the import statement:
+from ovos_workshop.skills.fallback import FallbackSkill class OVOSFallbackSkill(OVOSSkill): def __new__(cls, *args, **kwargs): log_deprecation("Implement " "`ovos_workshop.skills.fallback.FallbackSkill`", "0.1.0") - from ovos_workshop.skills.fallback import FallbackSkill return FallbackSkill(*args, **kwargs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- ovos_workshop/skills/ovos.py (1 hunks)
- test/unittests/test_skill.py (3 hunks)
🧰 Additional context used
🔇 Additional comments (5)
test/unittests/test_skill.py (2)
8-8: LGTM: Import statement updated correctlyThe removal of
is_classic_corefrom the import statement is in line with the PR objective of deprecating classic core support.
Line range hint
1-214: Overall changes are consistent and focusedThe modifications in this file are well-aligned with the PR objective of deprecating classic core support. The changes are focused on removing
is_classic_corechecks and adding OVOS-specific assertions. The rest of the file, including tests for other skill functionalities, remains intact, which maintains good overall test coverage.These changes effectively transition the test suite to assume an OVOS-only environment, which is consistent with the project's direction. The simplification of test logic should make future maintenance easier.
ovos_workshop/skills/ovos.py (3)
58-60: Deprecation ofis_classic_core()function is appropriateThe
is_classic_core()function is now deprecated and always returnsFalse, which aligns with the removal of support for classic mycroft-core. This change is appropriate and maintains backward compatibility.
Line range hint
316-322: Deprecation ofreport_metricmethod is appropriateThe
report_metricmethod is properly marked as deprecated in favor ofMetricsApi().report_metric. This encourages developers to use the updated API, ensuring consistency across the codebase.
Line range hint
324-330: Deprecation ofsend_emailmethod is appropriateThe
send_emailmethod is correctly deprecated, guiding developers to useEmailApi().send_email. This change aligns with the new API structure and improves code maintainability.
OVOS no longer supports running OVOS skills under classic mycroft-core
JarbasAl
force-pushed
the
deprecate/is_classic_core
branch
from
6953423 to
e8a48c3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #242 +/- ##
==========================================
- Coverage 53.41% 49.15% -4.27%
==========================================
Files 37 37
Lines 4362 4356 -6
==========================================
- Hits 2330 2141 -189
- Misses 2032 2215 +183 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (10)
ovos_workshop/skills/ovos.py (10)
Line range hint
95-97: Update the docstring in_OVOSSkillMetaclass.The docstring references
MycroftSkilland classic Mycroft Core. Given that support for classic Mycroft Core has been dropped, consider updating the documentation to reflect the current state of the codebase.
Line range hint
247-259: Clarify the defaultRuntimeRequirements.The docstring suggests that the default assumes network and internet are required. However, the method returns
RuntimeRequirements()without any parameters, which sets all requirements toFalse. This might lead to confusion.Apply this diff to set the correct default requirements:
@classproperty def runtime_requirements(self) -> RuntimeRequirements: """ Override to specify what a skill expects to be available at init and at runtime. Default will assume network and internet are required and GUI is not required for backwards-compat. """ - return RuntimeRequirements() + return RuntimeRequirements( + requires_network=True, + requires_internet=True, + no_network_fallback=True, + no_internet_fallback=False + )
Line range hint
836-839: Remove the deprecatednetwork_requirementsproperty.The
network_requirementsproperty is marked as deprecated and suggests usingruntime_requirementsinstead. Since this property is no longer in use, it's a good opportunity to remove it and clean up the code.
Line range hint
845-848: Make_voc_cachea read-only property.The setter for
voc_match_cachewarns against external modifications. To enforce this, consider removing the setter to make_voc_cacheread-only and prevent unintended external changes.Apply this diff to remove the setter:
@property def voc_match_cache(self) -> Dict[str, List[str]]: """ Backwards-compatible accessor method for vocab cache @return: dict vocab resources to parsed resources """ return self._voc_cache -@voc_match_cache.setter -def voc_match_cache(self, val): - self.log.warning("self._voc_cache should not be modified externally. This" - "functionality will be deprecated in a future release") - if isinstance(val, dict): - self._voc_cache = val
Line range hint
850-866: Remove deprecated translation methods.The methods
translate,translate_namedvalues,translate_list, andtranslate_templateare all deprecated. Since they redirect to the new implementations, it's advisable to remove these methods to reduce code redundancy.
Line range hint
873-881: Simplify theOVOSFallbackSkillclass definition.The
OVOSFallbackSkillclass is deprecated and currently returns an instance ofFallbackSkillusing the__new__method. For clarity and simplicity, consider making it an explicit alias or removing it if it's no longer necessary.Apply this diff to simplify the class:
-class OVOSFallbackSkill(OVOSSkill): - def __new__(cls, *args, **kwargs): - log_deprecation("Implement " - "`ovos_workshop.skills.fallback.FallbackSkill`", - "0.1.0") - from ovos_workshop.skills.fallback import FallbackSkill - return FallbackSkill(*args, **kwargs) +from ovos_workshop.skills.fallback import FallbackSkill as OVOSFallbackSkill
Line range hint
93-97: Remove backward compatibility checks for classic core in_OVOSSkillMetaclass.The
__instancecheck_classic__method is designed for backward compatibility with classic Mycroft Core. Since support for classic core has been deprecated, consider removing this method and related compatibility decorators to simplify the metaclass.
Line range hint
125-129: Clean up deprecated initialization warnings.In the
__init__method, there are warnings about missingbusandskill_idparameters for backward compatibility. Since the codebase is moving away from supporting classic Mycroft Core, consider removing these warnings and enforcing the required parameters.
Line range hint
539-542: Remove deprecated code related to skill GUI initialization.The
_init_skill_guimethod checks ifself.guiisNonebefore initializing it. Given that the GUI is now an integral part of the skill, consider enforcing its initialization and removing checks for backward compatibility.
Line range hint
618-623: Update the_register_public_apimethod to remove deprecated handling.The method contains code related to backward compatibility, such as handling functions without the
__name__attribute. Since support for older versions is deprecated, you can simplify this method by removing outdated checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- ovos_workshop/skills/ovos.py (1 hunks)
- test/unittests/test_skill.py (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unittests/test_skill.py
🧰 Additional context used
OVOS no longer supports running OVOS skills under classic mycroft-core
this gets rid of some warning logs about mycroft module deprecation
Summary by CodeRabbit
New Features
SkillGUIclass for enhanced GUI integration.voc_match_cacheproperty for vocabulary cache access.Deprecations
is_classic_core,report_metric, andsend_email.network_requirementstoruntime_requirements.Bug Fixes