fix: drop lingua-franca #26
Conversation
|
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request primarily focus on the Changes
Possibly related PRs
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 (16)
util/locale.py (1)
Line range hint
307-321: Optimize regex pattern for vocabulary matching.The current regex pattern can be optimized for better performance and security:
- Remove unnecessary
.*at start and end- Escape word list items to prevent regex injection
Consider this optimization:
- return any([re.match(r".*\b" + i + r"\b.*", utterance.lower()) for i in words]) + return any([re.search(r"\b" + re.escape(i) + r"\b", utterance.lower()) for i in words])util/parse_utils.py (1)
756-765: Consider standardizing docstring indentation.The docstring indentation is inconsistent between the two functions. Consider standardizing it for better readability.
-def _get_and_word(lang): - """ Helper to get word translations - - Args: - lang (str, optional): an optional BCP-47 language code, if omitted - the default language will be used. - - Returns: - str: translated version of resource name - """ +def _get_and_word(lang): + """Helper to get word translations + + Args: + lang (str, optional): an optional BCP-47 language code, if omitted + the default language will be used. + + Returns: + str: translated version of resource name + """Also applies to: 782-798
__init__.py (10)
281-282: Consider removing commented-out code.The commented-out decorator
# @killable_intent()is present above thehandle_create_alarmmethod. If this decorator is no longer needed, consider removing it to maintain clean and readable code.
Line range hint
334-345: Avoid usingassertstatements for runtime checks in production code.Using
assertfor checkingalarm.media_type == "ocp"can be problematic becauseassertstatements are removed when Python is run with optimization flags (e.g.,python -O). This could lead to the code not performing the necessary checks in production.Apply this diff to replace
assertwith explicit exception handling:alarm.ocp_request = ocp_result - assert alarm.media_type == "ocp" + if alarm.media_type != "ocp": + raise ValueError("Expected alarm media_type to be 'ocp'") self.confirm_alert(alarm, message)
372-372: Consider removing commented-out code.The commented-out decorator
# @killable_intent()is present above thehandle_create_remindermethod. Removing unused code keeps the codebase clean.
394-394: Consider removing commented-out code.The line
# if response and response != "no":is commented out. If this code is no longer necessary, consider removing it to improve code readability.
496-497: Remove unnecessary backslash for line continuation.When using parentheses for line continuation, backslashes are not needed. Removing them improves code readability.
Apply this diff:
rescheduled_time = \ - parse_relative_time_from_message(message, - anchor_time=anchor_time) + parse_relative_time_from_message( + message, anchor_time=anchor_time)
508-508: Remove unnecessary backslash for line continuation.In the conditional statement, the backslash is unnecessary due to the use of parentheses.
Apply this diff:
- if alert.has_repeat and \ - self.ask_yesno("reschedule_recurring_ask", - {"type": spoken_type}) == "yes": + if alert.has_repeat and self.ask_yesno( + "reschedule_recurring_ask", {"type": spoken_type}) == "yes": once = False
511-513: Remove unnecessary backslash for line continuation.Backslashes can be omitted when the line is enclosed in parentheses.
Apply this diff:
rescheduled = \ - self.alert_manager.reschedule_alert(alert, - rescheduled_time, - once) + self.alert_manager.reschedule_alert( + alert, rescheduled_time, once)
1859-1863: Simplify comparison by using the!=operator.In line 1861, the condition
if not to_play == "ocp":can be simplified toif to_play != "ocp":for better readability and adherence to Python conventions.Apply this diff:
# reset volume - if not to_play == "ocp": + if to_play != "ocp": self.bus.emit(Message("mycroft.volume.set", {"percent": self.original_volume}))🧰 Tools
🪛 Ruff
1861-1861: Use
to_play != "ocp"instead ofnot to_play == "ocp"Replace with
!=operator(SIM201)
Line range hint
1885-1894: Ensure consistent language settings when speaking dialogs.In the
_speak_notify_expiredmethod, consider specifying thelangparameter inself.speak_dialogcalls to ensure that the spoken language matches the user's settings.
Line range hint
1250-1272: Handle potential exceptions when accessing alert attributes.In the
conversemethod, when iterating overactivealerts, ensure that the alerts are checked forNonebefore accessing their attributes to prevent potentialAttributeErrorexceptions.Apply this diff:
for utterance in message.data.get("utterances"): # dismiss if voc_match(utterance, "dismiss", self.lang, exact=True): for alert in active: + if alert is None: + continue self._dismiss_alert(alert.ident, speak=True) return True # snoozetest/test_skill.py (4)
Line range hint
101-102: Avoid skipping the entireTestSkillclassThe
@unittest.skip('Work in progress')decorator skips all tests inTestSkill. This may prevent discovery of critical issues. Consider enabling the tests or using selective skips on individual tests that are not yet ready.Apply this diff to re-enable the test class:
-@unittest.skip('Work in progress')
Line range hint
132-138: Consider avoiding mocking protected methodsMocking protected methods like
_get_response_cascademay lead to brittle tests. Consider refactoring the code to mock public interfaces or using dependency injection to improve testability.
Line range hint
270-271: Address the TODO for testinguse_24hourpropertyThe test for
self.skill.use_24hourincludes a TODO comment suggesting further improvements are needed. Consider enhancing this test to cover more cases.Would you like assistance in improving this test case?
Line range hint
274-281: Useunittest.mock.patchto mock methods in testsInstead of manually saving and restoring
self.skill.confirm_alert, consider usingunittest.mock.patchto mockconfirm_alertwithin the test scope. This approach is cleaner and ensures the original method is restored even if the test fails.Apply this diff to refactor the test:
-def test_handle_create_alarm(self): - real_confirm = self.skill.confirm_alert - confirm_alert = Mock() - self.skill.confirm_alert = confirm_alert +@patch.object(AlertSkill, 'confirm_alert') +def test_handle_create_alarm(self, mock_confirm_alert): valid_message = _get_message_from_file("create_alarm_daily.json") invalid_message = _get_message_from_file( "invalid_messages/create_alarm_no_time.json" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
__init__.py(70 hunks)requirements.txt(1 hunks)test/test_skill.py(1 hunks)util/locale.py(1 hunks)util/parse_utils.py(3 hunks)util/ui_models.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- requirements.txt
🧰 Additional context used
🪛 Ruff
__init__.py
1861-1861: Use to_play != "ocp" instead of not to_play == "ocp"
Replace with != operator
(SIM201)
🔇 Additional comments (23)
util/ui_models.py (1)
31-31: LGTM! Verify function compatibility.
The import change from lingua_franca.format to ovos_date_parser aligns with the PR objective of dropping the lingua-franca dependency.
Let's verify that the new imports provide compatible implementations:
✅ Verification successful
Import change verified and compatible
The verification shows:
- The functions are used consistently throughout
ui_models.pywith the same parameter patterns:nice_durationis used withspeech=Falsenice_timeis used withspeech=False, use_ampm=not use_24h, use_24hour=use_24h
- No remaining
lingua_francaimports were found in the codebase - The change is isolated to this file and maintains the same function signatures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that ovos_date_parser provides compatible nice_duration and nice_time functions
# Test 1: Check if the functions exist in ovos_date_parser with the same parameters
ast-grep --pattern 'def nice_duration($$$, speech=$$$)'
ast-grep --pattern 'def nice_time($$$, speech=$$$, use_ampm=$$$, use_24hour=$$$)'
# Test 2: Check for any other files that might still be using lingua_franca.format
rg "from lingua_franca.format import"
Length of output: 181
Script:
#!/bin/bash
# Let's try a different approach to verify the usage and compatibility
# Check how nice_duration and nice_time are used in ui_models.py
rg -A 3 "nice_duration|nice_time" util/ui_models.py
# Check for any remaining lingua_franca imports in the entire codebase
rg "lingua_franca"
# Look for the actual implementation or usage patterns
rg -g "*.py" "def (nice_duration|nice_time)"
Length of output: 921
util/locale.py (3)
8-16: LGTM: Import changes align with PR objective.
The replacement of lingua_franca.format with ovos_date_parser and other OVOS utilities is appropriate and maintains the required functionality.
Line range hint 63-64: LGTM: Function signature improvements.
The addition of default values for the lang and anchor_date parameters improves function flexibility while maintaining backward compatibility.
Also applies to: 91-92, 127-128, 211-213
Line range hint 211-304: Verify timezone handling consistency.
While the timezone handling improvements are good, please ensure consistent timezone handling between expired_time and anchor_date. Currently, there could be inconsistencies if they have different timezone information.
Consider normalizing the timezones:
if anchor_date is None:
anchor_date = dt.datetime.now(expired_time.tzinfo)
+ else:
+ # Ensure anchor_date has the same timezone as expired_time
+ if anchor_date.tzinfo != expired_time.tzinfo:
+ anchor_date = anchor_date.astimezone(expired_time.tzinfo)util/parse_utils.py (3)
37-38: LGTM! Successfully replaced lingua-franca dependencies.
The replacement of lingua_franca imports with ovos_date_parser, ovos_number_parser, and UtteranceNormalizerPlugin maintains the required functionality while removing the dependency.
Also applies to: 42-42
141-143: LGTM! Enhanced tokenization with language-specific normalization.
The integration of UtteranceNormalizerPlugin provides proper language-specific text normalization while maintaining the existing functionality.
756-780: LGTM! Well-implemented internationalization support.
The new utility functions provide robust internationalization support with proper error handling and comprehensive language coverage.
Also applies to: 782-811
__init__.py (11)
30-31: Added necessary imports for time-related operations.
The imports of time, datetime, and timedelta are appropriate and necessary for time and date handling within the skill.
37-40: Added imports for date and number parsing utilities.
The imports from ovos_date_parser, ovos_number_parser, and ovos_skill_alerts.util provide essential functions for handling date and number formatting, which enhance the skill's capabilities.
42-42: Imported AlertManager and synchronization lock for alert management.
The import of AlertManager and SYNC_LOCK is crucial for managing alerts and ensuring thread safety during synchronization.
54-58: Imported media utilities for handling OpenVoiceOS Common Play integration.
The addition of ocp_query, ocp_request, and get_media_source_from_intent from ovos_skill_alerts.util.media enables the skill to interact with OCP for advanced media playback features.
78-79: Included GUI data builder and utility functions.
The import of build_gui_data and utility functions like create_daemon and classproperty supports the enhanced GUI interactions and background processes.
80-86: Added necessary imports for logging, runtime requirements, sound playback, and skill functionalities.
The imports from ovos_utils, ovos_utterance_normalizer, and ovos_workshop are essential for logging, defining runtime requirements, playing audio, normalizing utterances, handling intents, and skill inheritance.
170-170: Added play_volume property for controlling audio playback volume.
The play_volume property retrieves and normalizes the playback volume from settings, enhancing the skill's configurability.
590-590: Type hint added for clarity.
The addition of the type hint alert: Alert improves code clarity and helps with static type checking.
622-623: Utilizing utterance normalizer for improved parsing accuracy.
The introduction of UtteranceNormalizerPlugin enhances the robustness of utterance processing by normalizing user input.
727-728: Adjusted intent handler for better intent matching.
The TimerStatus intent now includes .one_of("time", "timer") and requires "remaining", improving the precision of intent recognition for timer status queries.
Line range hint 1279-1293:
test/test_skill.py (5)
39-40: Imports updated to use ovos_date_parser and ovos_number_parser
The imports have been correctly updated to replace lingua_franca.format, aligning with the dependency change.
Line range hint 57-57: Added necessary imports for locale configuration
The imports for load_language and get_default_tz are correctly added to support locale settings used in the tests.
Line range hint 122-129: Initializing AlertSkill with test settings
The AlertSkill is correctly instantiated with the test settings and alert paths for testing purposes.
Line range hint 143-143: Loading language configuration for tests
The call to load_language("en-us") ensures that language configurations are loaded, which is necessary for locale-dependent operations in the tests.
Line range hint 247-253: Resetting mocks in tearDown to ensure test isolation
The mocks are appropriately reset in the tearDown method, preventing interference between test cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
util/locale.py (1)
303-305: Consider simplifying the list joining expressionWhile the change to
join_word_listis good for localization, the expression could be more readable.Consider this simpler format:
- data["repeat"] = join_word_list([spoken_weekday(day, lang) - for day in alert.repeat_days], - connector="and", sep=",", lang=lang) + weekdays = [spoken_weekday(day, lang) for day in alert.repeat_days] + data["repeat"] = join_word_list(weekdays, connector="and", sep=",", lang=lang)util/parse_utils.py (2)
140-141: LGTM: Clean implementation of the new normalizerThe implementation correctly uses the language-specific normalizer from
UtteranceNormalizerPlugin.Consider caching the normalizer instance if this function is called frequently with the same language:
+_normalizer_cache = {} def tokenize_utterance(message: Message) -> Tokens: # ... existing code ... - normalizer = UtteranceNormalizerPlugin.get_normalizer(lang=lang) + if lang not in _normalizer_cache: + _normalizer_cache[lang] = UtteranceNormalizerPlugin.get_normalizer(lang=lang) + normalizer = _normalizer_cache[lang] tokens = Tokens([normalizer.normalize(chunk).lower() for chunk in chunks if chunk.strip()], message) return tokens
Line range hint
184-184: Update TODO comment to reflect new dependenciesThe TODO comment references "LF" (lingua-franca) which is being removed. This should be updated to use the new dependencies.
- # TODO ordinalize (LF) + # TODO: Implement ordinalization using ovos_date_parser__init__.py (2)
1843-1845: Simplify condition by using '!=' instead of 'not =='.The condition
if not to_play == "ocp":can be made more readable by using!=:Apply this diff to improve readability:
- if not to_play == "ocp": + if to_play != "ocp":🧰 Tools
🪛 Ruff
1845-1845: Use
to_play != "ocp"instead ofnot to_play == "ocp"Replace with
!=operator(SIM201)
283-283: Remove commented-out decorator to clean up the code.The
@killable_intent()decorator is commented out. If it's no longer needed, consider removing it to keep the codebase clean. If it's temporarily disabled, please add a comment explaining why.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
MANIFEST.in(1 hunks)__init__.py(63 hunks)requirements.txt(1 hunks)util/locale.py(4 hunks)util/parse_utils.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- MANIFEST.in
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 Additional context used
🪛 Ruff
__init__.py
1845-1845: Use to_play != "ocp" instead of not to_play == "ocp"
Replace with != operator
(SIM201)
🔇 Additional comments (7)
util/locale.py (4)
8-19: LGTM: Import changes align with dropping lingua-franca dependency
The replacement of lingua_franca with ovos_date_parser and other OVOS utilities is appropriate. The imported functions provide equivalent functionality for date/time formatting and list joining operations.
27-31: LGTM: Consistent datetime display logic
The datetime display logic remains correct with proper handling of date ranges and time formats.
139-144: LGTM: Proper timezone handling in duration calculations
The anchor time calculation correctly preserves timezone information from the alert time, ensuring accurate duration calculations.
Line range hint 1-329: Verify complete removal of lingua-franca
Let's ensure all lingua-franca references have been removed from the codebase.
✅ Verification successful
Since the search results show no instances of "lingua-franca" in either Python files or requirements.txt, and the code changes show consistent usage of OVOS date parsing utilities instead of lingua-franca, I can generate the final response.
All lingua-franca dependencies successfully removed ✓
The codebase has been fully migrated from lingua-franca to OVOS date parsing utilities. All imports and function calls have been updated consistently, with no remaining references to lingua-franca.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining lingua-franca references
echo "Checking for remaining lingua-franca imports..."
rg "lingua.?franca" --type py
echo "Checking for any lingua-franca in requirements..."
rg "lingua.?franca" requirements.txt || true
Length of output: 291
util/parse_utils.py (1)
37-38: LGTM: Clean replacement of lingua-franca dependencies
The replacement of lingua_franca with ovos_date_parser, ovos_number_parser, and UtteranceNormalizerPlugin maintains equivalent functionality while removing the dependency.
Also applies to: 50-50
__init__.py (2)
352-352: Remove commented-out decorator to clean up the code.
The @killable_intent() decorator is commented out here as well. For consistency and code cleanliness, consider removing it or providing a comment explaining its deactivation.
614-615: Ensure proper handling of 'utterance' extraction.
When accessing message.data.get("utterance", ""), ensure that 'utterance' exists in message.data to prevent potential issues. If 'utterance' might not be present, consider adding a check or default handling.
Run the following script to verify the presence of 'utterance' in message.data across the codebase:
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (11)
util/alert.py (1)
109-109: Consider enhancing thelangproperty implementation.The implementation looks good and follows the class's property patterns. Consider these enhancements:
- Add a setter method to allow language updates
- Document the expected language code format (e.g., 'en-US', 'de-DE')
Example implementation:
@property def lang(self) -> str: """ - Returns the lang associated with this alert + Returns the language code associated with this alert + :returns: Language code (e.g., 'en-US', 'de-DE') """ return self._data.get("lang") or get_default_lang() +@lang.setter +def lang(self, lang_code: str) -> None: + """ + Sets the language code for this alert + :param lang_code: Language code (e.g., 'en-US', 'de-DE') + """ + self._data["lang"] = lang_codeutil/parse_utils.py (4)
67-74: Optimize the unmatched method using a ternary operatorThe code can be simplified using a ternary operator for better readability.
def unmatched(self, original=False): - if original: - list_ = self.original - else: - list_ = self + list_ = self.original if original else self return [chunk for chunk in list_ if not (self.is_matched(chunk) or chunk in get_words_list("noise_words.voc", self.lang))]🧰 Tools
🪛 Ruff
68-71: Use ternary operator
list_ = self.original if original else selfinstead ofif-else-blockReplace
if-else-block withlist_ = self.original if original else self(SIM108)
667-675: Add more specific type hints for validation functionsThe type hints could be more specific to improve code maintainability and IDE support.
-def validate_dt_or_delta(response: str, lang: str = None) -> Union[dt.datetime, dt.timedelta, str]: +def validate_dt_or_delta(response: str, lang: Optional[str] = None) -> Union[dt.datetime, dt.timedelta, Literal["no"]]:
732-733: Add structured logging for fuzzy matchingThe commented debug log could be improved with structured logging to help with debugging and monitoring.
- # LOG.debug(f"Fuzzy Match value {best}") + LOG.debug("Fuzzy match results", extra={ + "ratio": ratio, + "partial_ratio": partial_ratio, + "token_sort_ratio": token_sort_ratio, + "best_match": best + }) -Also applies to: 736-736
Action Required: The TODO regarding timezone in the signature is still present in
util/parse_utils.py. Please address or remove it as appropriate.🔗 Analysis chain
Line range hint
417-424: Review and remove TODO comment if no longer relevantThere's a TODO comment about timezone testing that seems to be partially implemented. Please verify if this is still relevant and either implement the remaining changes or remove the comment if it's no longer needed.
Would you like me to help implement the timezone testing or create an issue to track this?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any related timezone issues in the test files rg -l "timezone" test/Length of output: 41
Script:
#!/bin/bash # Check if the TODO comment about timezone testing is still present in util/parse_utils.py rg "TODO.*timezone" util/parse_utils.pyLength of output: 87
test/test_skill.py (3)
454-455: Consider using message language for time formattingThe language parameter is hardcoded to "en-us" in several nice_time/nice_date_time calls. For better internationalization support, consider using the language from the message context instead.
Example refactor:
- nice_time(self.valid_event.expiration, lang="en-us") + nice_time(self.valid_event.expiration, lang=message.data.get("lang", "en-us"))Also applies to: 472-472, 721-721, 796-797, 808-809, 821-823, 835-836
1482-1485: Add missing test coverage for skipped test methodsSeveral test methods are marked as TODO and skipped:
- test_alert_expired
- test_play_notify_expired
- test_speak_notify_expired
- test_gui_timer_status
- test_gui_notify_expired
- test_resolve_requested_alert
- test_get_events
- test_get_requested_alert_name_and_time
- test_get_alert_type_from_intent
Consider implementing these tests to ensure complete coverage of the alert functionality.
Would you like me to help create test cases for these methods?
Also applies to: 1504-1506, 1527-1529, 1556-1558, 1587-1589, 1610-1612, 1629-1633, 1654-1656
Skipped Test Classes Could Lead to Missing Coverage
Several test classes are marked with @unittest.skip('Work in progress'). These skipped classes could lead to missing test coverage. Consider either:
- Implementing the test classes if the functionality exists
- Removing them if they are no longer needed
- Adding TODO comments explaining why they are skipped
🔗 Analysis chain
Line range hint
1-24: Consider implementing or removing skipped test classesSeveral test classes are marked with @unittest.skip('Work in progress'):
- TestAlert
- TestAlertManager
- TestParseUtils
- TestUIModels
- TestSkillLoading
- TestSkillIntentMatching
These skipped classes could lead to missing test coverage. Consider either:
- Implementing the test classes if the functionality exists
- Removing them if they are no longer needed
- Adding TODO comments explaining why they are skipped
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for skipped tests and their impact on coverage rg -A 2 "@unittest.skip" test/test_skill.pyLength of output: 818
util/locale.py (1)
8-8: Remove unused importload_languageto clean up the code.The
load_languagefunction imported fromovos_config.localeis not used in the codebase and can be safely removed to enhance code clarity.Apply this diff to remove the unused import:
-from ovos_config.locale import get_default_lang, load_language +from ovos_config.locale import get_default_lang🧰 Tools
🪛 Ruff
8-8:
ovos_config.locale.load_languageimported but unusedRemove unused import:
ovos_config.locale.load_language(F401)
__init__.py (2)
Line range hint
170-175: Incorrect return type annotation inplay_volumepropertyThe
play_volumeproperty returns a float value (e.g., between 0 and 1), but the return type annotation specifies-> int, which is incorrect. This should be corrected to-> floatto accurately reflect the return value and avoid potential type errors.Apply this diff to fix the return type annotation:
- def play_volume(self) -> int: + def play_volume(self) -> float:
283-284: Remove commented-out decoratorThe
@killable_intent()decorator is commented out. If it's no longer needed, consider removing it to clean up the code.Apply this diff to remove the commented code:
- #@killable_intent()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
__init__.py(65 hunks)setup.py(1 hunks)test/test_skill.py(16 hunks)util/alert.py(2 hunks)util/locale.py(8 hunks)util/parse_utils.py(21 hunks)util/ui_models.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- util/ui_models.py
🧰 Additional context used
🪛 Ruff
__init__.py
1846-1846: Use to_play != "ocp" instead of not to_play == "ocp"
Replace with != operator
(SIM201)
util/locale.py
8-8: ovos_config.locale.load_language imported but unused
Remove unused import: ovos_config.locale.load_language
(F401)
util/parse_utils.py
68-71: Use ternary operator list_ = self.original if original else self instead of if-else-block
Replace if-else-block with list_ = self.original if original else self
(SIM108)
🔇 Additional comments (14)
setup.py (1)
93-93: Verify migration of vocab/dialog resources
The removal of "vocab" and "dialog" directories from resource_base_dirs aligns with dropping lingua-franca. However, let's verify that these resources have been properly migrated or are no longer needed.
✅ Verification successful
Resources properly migrated to locale directory
All dialog and vocab files have been successfully migrated to language-specific directories under locale/. The new structure properly organizes resources while maintaining all functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if vocab/dialog resources were migrated to other locations
# Check if old directories still exist and contain files
echo "Checking for any remaining files in old resource directories..."
find . -type f \( -path "*/vocab/*" -o -path "*/dialog/*" \)
# Check if resources were migrated to new locations
echo "Checking for potential migrated resources..."
find . -type f \( -path "*/locale/*" -o -path "*/res/*" \) -newer "$(git log --format=%ct -1 HEAD^)^%ct"
Length of output: 98796
Script:
#!/bin/bash
# Description: Verify if vocab/dialog resources were migrated to other locations
# Check if old directories still exist and contain files
echo "Checking for any remaining files in old resource directories..."
find . -type f \( -path "*/vocab/*" -o -path "*/dialog/*" \)
# Get the timestamp of the previous commit
PREV_COMMIT_TIMESTAMP=$(git log --format=%ct -1 HEAD^)
echo "Previous commit timestamp: $PREV_COMMIT_TIMESTAMP"
# Check if resources were migrated to new locations after the previous commit
echo "Checking for potential migrated resources..."
find . -type f \( -path "*/locale/*" -o -path "*/res/*" \) -newermt "@$PREV_COMMIT_TIMESTAMP"
Length of output: 197845
util/alert.py (1)
39-39: LGTM!
The import statement is correctly placed and properly groups related functionality from the ovos_config.locale module.
util/parse_utils.py (1)
37-38: LGTM: Successfully migrated from lingua-franca to OVOS libraries
The migration from lingua-franca to ovos_date_parser and ovos_number_parser is implemented correctly.
__init__.py (11)
30-33: Approved: Added necessary import statements
The additional import statements for time, RLock, and typing annotations are appropriate for the functionality added in the code.
Line range hint 101-109: Approved: Added runtime_requirements class property
The runtime_requirements property correctly specifies the runtime requirements for the skill, ensuring proper behavior without unnecessary dependencies.
276-279: Approved: Corrected dialog data formatting
The changes to the speak_dialog call improve the readability of the code and ensure that the join_word_list function is used correctly with the specified parameters.
414-423: Approved: Improved handling of overlapping alerts
The logic for managing overlapping alerts has been enhanced to provide better user interaction and clarity in dialog responses.
736-738: Approved: Naming unnamed timers for user clarity
Assigning ordinal numbers to unnamed timers when multiple timers are active improves user experience by providing a reference when interacting with timers.
902-905: Approved: Proper handling of empty todo lists
The check for empty todos and appropriate user feedback enhances the robustness of the skill when no todo lists are present.
1052-1054: Approved: Gathering user responses for items to delete
The use of _get_response_cascade to collect items for deletion provides an interactive way for users to specify which items to remove.
Line range hint 1701-1704: Approve: Corrected method for deactivating the skill
The handle_deactivate method correctly ensures that the skill remains active if there are active alerts, preventing premature deactivation.
1919-1923: Approved: Handling of volume reset for timers
The code correctly resets the volume after handling timer alerts, ensuring user settings are maintained.
1969-1972: Approved: Skill deactivation when no active alerts
The skill appropriately deactivates itself when there are no more active alerts, freeing up resources.
Line range hint 778-784: Verify deletion of alerts in cancellation loop
When cancelling all alerts, ensure that the loop correctly handles the deletion of alerts to prevent potential issues with iterating over a changing list.
Run the following script to verify that alerts are properly cancelled without errors:
✅ Verification successful
Confirmed that alerts are properly cancelled within the cancellation loop.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check that alerts are properly removed during cancellation
# Test: Attempt to cancel multiple alerts and ensure no runtime errors occur
python -c '
from __init__ import AlertSkill, AlertType, AlertState
skill = AlertSkill()
skill.handle_cancel_alert(Message(""))
'
Length of output: 204
Script:
#!/bin/bash
# Description: Check that alerts are properly removed during cancellation
# Test: Attempt to cancel multiple alerts and ensure no runtime errors occur
python3 -c '
from __init__ import AlertSkill, AlertType, AlertState
from some_module import Message # Ensure Message is correctly imported
skill = AlertSkill()
skill.handle_cancel_alert(Message(""))
'
Length of output: 278
Script:
#!/bin/bash
# Description: Verify that alerts are properly deleted in the cancellation loop to prevent issues with iterating over a changing list
# Search for loops that modify the alerts list during iteration in __init__.py
rg --multiline '(for\s+\w+\s+in\s+\w+:\s*\n\s+.*\.remove\(\w+\))' __init__.py
Length of output: 79
| time_, remainder = extract_duration(text, lang=lang) | ||
| if time_ is None: | ||
| #now = dt.datetime.now(timezone) | ||
| time_, remainder = extract_datetime(text, anchorDate=anchor_time) or (None, text) | ||
| if isinstance(time_, (dt.timedelta, relativedelta)): | ||
| # now = dt.datetime.now(timezone) | ||
| time_, remainder = extract_datetime(text, anchorDate=anchor_time, lang=lang) or (None, text) | ||
| if isinstance(time_, (dt.timedelta, relativedelta)): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in extract_dt_or_duration
The error handling could be improved by:
- Adding explicit error handling for the extract_duration call
- Adding type hints for the return values
time_, remainder = extract_duration(text, lang=lang)
if time_ is None:
- time_, remainder = extract_datetime(text, anchorDate=anchor_time, lang=lang) or (None, text)
+ try:
+ extracted = extract_datetime(text, anchorDate=anchor_time, lang=lang)
+ time_, remainder = extracted if extracted else (None, text)
+ except Exception as e:
+ LOG.warning(f"Error parsing datetime: {e}")
+ time_, remainder = None, text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| time_, remainder = extract_duration(text, lang=lang) | |
| if time_ is None: | |
| #now = dt.datetime.now(timezone) | |
| time_, remainder = extract_datetime(text, anchorDate=anchor_time) or (None, text) | |
| if isinstance(time_, (dt.timedelta, relativedelta)): | |
| # now = dt.datetime.now(timezone) | |
| time_, remainder = extract_datetime(text, anchorDate=anchor_time, lang=lang) or (None, text) | |
| if isinstance(time_, (dt.timedelta, relativedelta)): | |
| time_, remainder = extract_duration(text, lang=lang) | |
| if time_ is None: | |
| try: | |
| extracted = extract_datetime(text, anchorDate=anchor_time, lang=lang) | |
| time_, remainder = extracted if extracted else (None, text) | |
| except Exception as e: | |
| LOG.warning(f"Error parsing datetime: {e}") | |
| time_, remainder = None, text | |
| if isinstance(time_, (dt.timedelta, relativedelta)): |
| @@ -1865,11 +1841,10 @@ def _play_notify_expired(self, alert: Alert): | |||
| play_audio(to_play).wait(60) | |||
| # TODO this depends on the length of the audio file | |||
| time.sleep(1) | |||
|
|
|||
|
|
|||
| # reset volume | |||
| if not to_play == "ocp": | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify condition by using != operator
Instead of using not to_play == "ocp", it is more Pythonic to use to_play != "ocp" for clarity and readability.
Apply this diff to simplify the condition:
- if not to_play == "ocp":
+ if to_play != "ocp":📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not to_play == "ocp": | |
| if to_play != "ocp": |
🧰 Tools
🪛 Ruff
1846-1846: Use to_play != "ocp" instead of not to_play == "ocp"
Replace with != operator
(SIM201)
| kinds = {(a.alert_type.value, spoken_alert_type(a.alert_type)) | ||
| for a in alerts_list if a.expiration is not None} | ||
| else: | ||
| kinds = {(alert_type.value, spoken_type)} | ||
|
|
There was a problem hiding this comment.
Potential issue with variable kinds in alert listing
In the handle_list_all_alerts method, when checking if message.data.get("alert"), the code collects kinds from all alerts, even if they may not match the intended alert type. Consider filtering alerts_list by the specified alert_type to ensure consistency.
Apply this diff to filter alerts by alert_type:
kinds = {(a.alert_type.value, spoken_alert_type(a.alert_type))
- for a in alerts_list if a.expiration is not None}
+ for a in alerts_list if a.alert_type == alert_type and a.expiration is not None}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kinds = {(a.alert_type.value, spoken_alert_type(a.alert_type)) | |
| for a in alerts_list if a.expiration is not None} | |
| else: | |
| kinds = {(alert_type.value, spoken_type)} | |
| kinds = {(a.alert_type.value, spoken_alert_type(a.alert_type)) | |
| for a in alerts_list if a.alert_type == alert_type and a.expiration is not None} | |
| else: | |
| kinds = {(alert_type.value, spoken_type)} |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (18)
util/locale.py (5)
Line range hint
23-61: Fix inconsistent parameter indentationThe function parameter indentation is inconsistent (4 vs 5 spaces). While the logic is correct, maintaining consistent indentation improves readability.
Apply this formatting fix:
-def datetime_display(begin: dt.datetime, - end: Optional[dt.datetime] = None, - lang: str = None) -> str: +def datetime_display(begin: dt.datetime, + end: Optional[dt.datetime] = None, + lang: str = None) -> str:The addition of the ValueError for invalid date formats is a good improvement in error handling.
Line range hint
74-90: Fix missing lang parameter in translate() callIn the TODO case, the lang parameter is missing in the translate() call, which could cause inconsistent language handling.
Apply this fix:
elif alert_type == AlertType.TODO: - return translate("todo") + return translate("todo", lang)
266-276: Fix inconsistent parameter indentation in time formatting callsThe indentation of parameters in nice_date_time and nice_time calls is inconsistent (5 spaces).
Apply this formatting fix:
- spoken_time = nice_date_time(expired_time, lang=lang, - use_24hour=use_24hour, - use_ampm=not use_24hour) + spoken_time = nice_date_time(expired_time, lang=lang, + use_24hour=use_24hour, + use_ampm=not use_24hour)
307-309: Fix inconsistent parameter indentation in join_word_list callThe indentation of parameters in the join_word_list call is inconsistent (5 spaces).
Apply this formatting fix:
- data["repeat"] = join_word_list([spoken_weekday(day, lang) - for day in alert.repeat_days], - connector="and", sep=",", lang=lang) + data["repeat"] = join_word_list([spoken_weekday(day, lang) + for day in alert.repeat_days], + connector="and", sep=",", lang=lang)
Line range hint
235-334: Consider breaking down get_alert_dialog_data for better maintainabilityWhile the function handles all cases correctly, its complexity suggests it could benefit from being broken down into smaller, focused helper functions for different types of alerts (all-day events, repeating events, etc.).
Would you like me to propose a refactored structure that improves maintainability while preserving all functionality?
util/parse_utils.py (4)
68-75: Consider simplifying the unmatched method using a list comprehensionThe method can be made more concise while maintaining readability.
def unmatched(self, original=False): - if original: - list_ = self.original - else: - list_ = self - return [chunk for chunk in list_ if - not (self.is_matched(chunk) or - chunk in get_words_list("noise_words.voc", self.lang))] + list_ = self.original if original else self + return [chunk for chunk in list_ + if not (self.is_matched(chunk) or + chunk in get_words_list("noise_words.voc", self.lang))]🧰 Tools
🪛 Ruff
69-72: Use ternary operator
list_ = self.original if original else selfinstead ofif-else-blockReplace
if-else-block withlist_ = self.original if original else self(SIM108)
Line range hint
467-483: Remove TODO comment and implement OCP for audio file handlingThe TODO comment indicates that audio file handling should be moved out of this utility file.
Would you like me to help create a separate audio file handler class that follows the Open-Closed Principle? This would improve the modularity of the code and make it easier to extend audio file handling in the future.
Line range hint
731-749: Optimize fuzzy matching for better performanceThe fuzzy matching implementation can be improved for better performance and maintainability.
def fuzzy_match_alerts(test: List[Alert], against: str, confidence: int = None) \ -> Optional[Alert]: - result = [] - for alert in test: - fuzzy_conf = fuzzy_match(alert.alert_name, against) - if confidence and fuzzy_conf > confidence: - result.append((alert, fuzzy_conf)) - if result: - return sorted(result, key=lambda x: x[1])[-1][0] - return None + matches = [(alert, fuzzy_match(alert.alert_name, against)) + for alert in test] + if confidence: + matches = [(a, c) for a, c in matches if c > confidence] + return max(matches, key=lambda x: x[1])[0] if matches else None
Line range hint
1-749: Overall assessment: Good changes with room for improvementThe migration from lingua-franca to ovos-specific parsers is well implemented. However, consider:
- Improving error handling across parsing functions
- Addressing the TODOs, especially for audio file handling
- Optimizing the fuzzy matching implementation
- Adding comprehensive unit tests for the new parsing utilities
The changes are functional but could benefit from these improvements for better maintainability and reliability.
__init__.py (4)
Line range hint
143-170: Improve error handling in timer_sound_file propertyThe property could raise FileNotFoundError even after trying the default path. Consider adding a final fallback or better error handling.
Apply this diff to improve error handling:
@property def timer_sound_file(self) -> str: """ Return the path to a valid timer sound resource file """ filename = self.settings.get('sound_timer') or 'default-timer.wav' if os.path.isfile(filename): return filename file = self.find_resource(filename) if not file: LOG.warning(f'Could not resolve requested file: {filename}') file = os.path.join(os.path.dirname(__file__), 'res', 'snd', 'default-timer.wav') - if not file: - raise FileNotFoundError(f"Could not resolve sound: {filename}") + if not os.path.isfile(file): + LOG.error(f"Could not resolve any timer sound file") + # Return a known system sound file as ultimate fallback + return "/usr/share/sounds/freedesktop/stereo/bell.oga" return file
Line range hint
1622-1626: Consider increasing thread lock timeoutThe current 1 second timeout for acquiring the GUI timer lock might be too short under high load, potentially causing missed updates.
Apply this diff:
- if not self._gui_timer_lock.acquire(True, 1): + if not self._gui_timer_lock.acquire(True, 5): # Increase timeout to 5 seconds
Line range hint
1779-1849: Ensure volume is always restored using try-finallyThe volume should be restored even if an exception occurs during playback.
Apply this diff:
def _play_notify_expired(self, alert: Alert): self.original_volume = self.bus.wait_for_response( Message("mycroft.volume.get") ) or 100 - secs_played = 0 - timeout = self.alert_timeout_seconds - # ... rest of the method ... - if not to_play == "ocp": - self.bus.emit(Message("mycroft.volume.set", - {"percent": self.original_volume})) + try: + secs_played = 0 + timeout = self.alert_timeout_seconds + # ... rest of the method ... + finally: + if not to_play == "ocp": + self.bus.emit(Message("mycroft.volume.set", + {"percent": self.original_volume}))🧰 Tools
🪛 Ruff
1847-1847: Use
to_play != "ocp"instead ofnot to_play == "ocp"Replace with
!=operator(SIM201)
Line range hint
2008-2012: Improve resource cleanup in shutdownConsider adding explicit cleanup of timers and locks to prevent resource leaks.
Apply this diff:
def shutdown(self): LOG.debug(f"Shutdown, all active alerts are now missed") + # Cancel any scheduled events + self.cancel_all_repeating_events() + # Release GUI timer lock if held + if self._gui_timer_lock.locked(): + self._gui_timer_lock.release() self.alert_manager.shutdown() self.gui.clear()test/test_skill.py (5)
Line range hint
1479-1586: Add test coverage for edge cases in dialog data generationThe test_get_alert_dialog_data method should include additional test cases:
- Alerts with invalid/missing timezones
- Edge cases around DST transitions
- Invalid repeat patterns
Consider adding test cases for these scenarios to improve coverage.
Line range hint
1607-1630: Fix inconsistent variable usage in test assertionsThe test is using
daily.expirationinstead ofweekly.expirationin the remaining time calculation.Apply this fix:
- "remaining": spoken_duration(daily.expiration, lang="en-us") + "remaining": spoken_duration(weekly.expiration, lang="en-us")
Line range hint
1651-1655: Add test coverage for timer name formattingThe test verifies basic timer name formatting but doesn't cover:
- Special characters in timer names
- Very long timer names
- Empty timer names
Consider adding test cases for these scenarios to improve coverage.
Line range hint
1-24: Update copyright headerThe copyright header contains outdated information and references to specific companies and patents.
Consider updating the header to reflect current ownership and licensing information.
Line range hint
718-794: Add test coverage for alert expiration handlingThe test_handle_event_timeframe_check method lacks coverage for:
- Expired alerts
- Alerts expiring during the check
- Multiple alerts with the same expiration time
Consider adding test cases for these scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
__init__.py(67 hunks)test/test_skill.py(16 hunks)util/alert.py(4 hunks)util/locale.py(10 hunks)util/parse_utils.py(27 hunks)util/ui_models.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- util/alert.py
- util/ui_models.py
🧰 Additional context used
🪛 Ruff
__init__.py
1847-1847: Use to_play != "ocp" instead of not to_play == "ocp"
Replace with != operator
(SIM201)
util/parse_utils.py
69-72: Use ternary operator list_ = self.original if original else self instead of if-else-block
Replace if-else-block with list_ = self.original if original else self
(SIM108)
🔇 Additional comments (10)
util/locale.py (2)
3-20: LGTM! Clean migration from lingua-franca to ovos packages
The import statements are well-organized and properly structured, successfully replacing lingua-franca dependencies with ovos-specific packages.
Line range hint 135-234: LGTM! Robust timezone and internationalization handling
The changes properly handle timezone-aware datetimes and consistently use language parameters across all functions. The code is well-documented with appropriate type hints.
util/parse_utils.py (2)
35-38: LGTM: Import changes align with dependency updates
The replacement of lingua-franca with ovos-specific parsers is implemented correctly.
599-603:
Improve error handling in extract_dt_or_duration
The function should handle potential exceptions from the parsing utilities.
- time_, remainder = extract_duration(text, lang=lang)
- if time_ is None:
- time_, remainder = extract_datetime(text, anchorDate=anchor_time, lang=lang) or (None, text)
+ try:
+ time_, remainder = extract_duration(text, lang=lang)
+ if time_ is None:
+ extracted = extract_datetime(text, anchorDate=anchor_time, lang=lang)
+ time_, remainder = extracted if extracted else (None, text)
+ except Exception as e:
+ LOG.warning(f"Error parsing time: {e}")
+ return None, textLikely invalid or redundant comment.
__init__.py (2)
37-38: LGTM: Successfully replaced lingua-franca with OVOS alternatives
The changes correctly replace lingua-franca date/time parsing functionality with equivalent OVOS modules.
1847-1847: 🛠️ Refactor suggestion
Simplify condition using != operator
Use != operator for better readability and maintainability.
Apply this diff:
- if not to_play == "ocp":
+ if to_play != "ocp":Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff
1847-1847: Use to_play != "ocp" instead of not to_play == "ocp"
Replace with != operator
(SIM201)
test/test_skill.py (4)
39-40: Update deprecated imports
The code imports from ovos_number_parser and ovos_date_parser which are newer replacements for lingua_franca.format. This is consistent with the PR objective to drop lingua-franca.
Line range hint 1-2000: Overall test structure and coverage is good
The test file provides comprehensive coverage of the alerts functionality including:
- Alert creation and management
- Timer and alarm handling
- UI models and display formatting
- Timezone and locale handling
- Calendar integration
46-46: Verify timezone configuration import
The code imports timezone configuration from ovos_config.locale. Verify this is the correct source for timezone information.
#!/bin/bash
# Check for other timezone configuration imports
rg "import.*timezone" 451-452: 🛠️ Refactor suggestion
Verify timezone handling in test assertions
The test uses nice_time and nice_date_time functions with hardcoded timezone parameters. This could lead to flaky tests when run in different timezones.
Consider using timezone-aware datetime objects and explicit timezone conversion in the test assertions:
- begin = nice_time(self.valid_event.expiration, lang="en-us")
+ begin = nice_time(self.valid_event.expiration.astimezone(tzlocal()), lang="en-us")Also applies to: 469-469, 793-794, 805-806, 818-820, 832-833
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
requirements.txtto includeovos-utterance-normalizer,ovos-number-parser, andovos-date-parser.Tests
Chores
MANIFEST.into include necessary files in the package distribution.setup.py.