Split date-time runtime fixes from locale PR#185
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds locale-aware phrase normalization and cached phrase sets, improves case-insensitive location extraction and preprocessing, treats locale-listed ambiguous locations as unresolved for timezone resolution, updates time methods to return Optional[str] and accept anchor_date, and refactors weekend calculation using weekday arithmetic and locale cues. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as "User (utterance)"
participant Skill as "TimeSkill"
participant Locale as "Locale resources"
participant TZ as "Timezone resolver"
User->>Skill: speak/query (utterance, optional location slot)
Skill->>Locale: _load_locale_phrase_set / _normalize_phrase
Locale-->>Skill: phrase sets (cached)
Skill->>Skill: _resolve_location(utterance, slot)
alt location ambiguous per locale
Skill->>Skill: _is_ambiguous_location -> return None
Skill-->>User: respond with unresolved location / fallback
else location resolved
Skill->>TZ: get_timezone_in_location(location)
TZ-->>Skill: timezone or None
Skill->>Skill: compute datetime (anchor_date if provided)
Skill-->>User: spoken/display time (or None)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Refactored the locale-specific exceptions out of This keeps the runtime fix generic and moves the data into optional per-locale
I added |
|
Split the locale resources into #186 so this PR stays code-only.\n\nThe locale data for ambiguous locations, non-location phrases, and current-weekend wording now lives there. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
__init__.py (3)
416-429: Use explicitOptionaltype hints for parameters.Both
locationandanchor_dateparameters have implicitOptionaltypes.The propagation of
anchor_datethrough the call chain and location sanitization logic are correctly implemented.Proposed fix
- def speak_time(self, dialog: str, location: str = None, - anchor_date: datetime.datetime = None): + def speak_time(self, dialog: str, location: Optional[str] = None, + anchor_date: Optional[datetime.datetime] = None):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 416 - 429, The speak_time method currently uses implicit Optional types for location and anchor_date; update its signature to use explicit typing (e.g., location: Optional[str], anchor_date: Optional[datetime.datetime]) and import typing.Optional if not already present, then propagate the explicit Optional type to calls and any related helper methods (get_spoken_time, get_display_time, and _sanitize_location) as needed to keep type consistency across the call chain and avoid type-checker warnings.
344-358: Use explicitOptionaltype hints.The parameter type hints use implicit
Optionalwhich is prohibited by PEP 484. The static analyzer flagged this.Proposed fix
def get_spoken_time(self, location: str = None, force_ampm=False, - anchor_date: datetime.datetime = None) -> Optional[str]: + anchor_date: Optional[datetime.datetime] = None) -> Optional[str]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 344 - 358, The function get_spoken_time currently uses None defaults without explicit Optional annotations for parameters (location: str = None and anchor_date: datetime.datetime = None); update these to use explicit Optional types (location: Optional[str] and anchor_date: Optional[datetime.datetime]) and ensure Optional is imported/available in the module, keeping the existing return type of Optional[str] and leaving force_ampm as bool.
370-384: Use explicitOptionaltype hint.Same implicit
Optionalissue asget_spoken_time.Proposed fix
def get_display_time(self, location: str = None, force_ampm=False, - anchor_date: datetime.datetime = None) -> Optional[str]: + anchor_date: Optional[datetime.datetime] = None) -> Optional[str]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 370 - 384, The get_display_time signature uses location: str = None which is implicitly allowing None; change the parameter typing to Optional[str] and mirror the explicit optional typing used for anchor_date (and adjust force_ampm typing if desired), e.g. update the function signature for get_display_time to use location: Optional[str] = None and anchor_date: Optional[datetime.datetime] = None while keeping the return type -> Optional[str]; ensure Optional is imported/available like in get_spoken_time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__init__.py`:
- Around line 416-429: The speak_time method currently uses implicit Optional
types for location and anchor_date; update its signature to use explicit typing
(e.g., location: Optional[str], anchor_date: Optional[datetime.datetime]) and
import typing.Optional if not already present, then propagate the explicit
Optional type to calls and any related helper methods (get_spoken_time,
get_display_time, and _sanitize_location) as needed to keep type consistency
across the call chain and avoid type-checker warnings.
- Around line 344-358: The function get_spoken_time currently uses None defaults
without explicit Optional annotations for parameters (location: str = None and
anchor_date: datetime.datetime = None); update these to use explicit Optional
types (location: Optional[str] and anchor_date: Optional[datetime.datetime]) and
ensure Optional is imported/available in the module, keeping the existing return
type of Optional[str] and leaving force_ampm as bool.
- Around line 370-384: The get_display_time signature uses location: str = None
which is implicitly allowing None; change the parameter typing to Optional[str]
and mirror the explicit optional typing used for anchor_date (and adjust
force_ampm typing if desired), e.g. update the function signature for
get_display_time to use location: Optional[str] = None and anchor_date:
Optional[datetime.datetime] = None while keeping the return type ->
Optional[str]; ensure Optional is imported/available like in get_spoken_time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9484dae6-7264-4b38-88f7-1f3dd7cb0d3d
📒 Files selected for processing (8)
__init__.pylocale/en-us/ambiguous_locations.listlocale/en-us/current_weekend_phrases.listlocale/en-us/non_location_phrases.listlocale/fr-fr/ambiguous_locations.listlocale/fr-fr/current_weekend_phrases.listlocale/fr-fr/non_location_phrases.listlocale/tr-tr/current_weekend_phrases.list
|
Hey @goldyfruit , a few items of feedback:
I know it's out of scope for this particular PR but it would be nice to see if the tests pass with your changes. |
|
Addressed the runtime cleanup feedback here:
|
Beep! Your PR results are served. 🍽️I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthEnsuring the repo isn't allergic to new features. 🤧 ✅ All required files present. Latest Version: ✅ 🎙️ SkillSkill structure analysis complete! 🧠 🎙️ ovos-skill-date-time.openvoiceos — 20 languages en-us: 12 intents · 18 dialogs · skill.json ✅ Translation coverage — 19 languages (1 complete, 12 partial, 6 incomplete)
🔍 LintChecking if we've met all our check criteria. ✅ ❌ ruff: issues found — see job log 🔌 Skill Tests (ovoscope)A comprehensive review of the skill's conversational flow. 🌊 ✅ 0/0 passed 🚌 Bus CoverageIs the message bus fully covered? Let's see the stats. 📊 🏷️ Release PreviewEnsuring the 'Dependency Updates' are documented. 📦 Current:
🚀 Release Channel Compatibility Predicted next version:
🔨 Build TestsThe build process has successfully terminated. 🏁 ✅ All versions pass
⚖️ License CheckChecking if the licenses are compatible with OVOS. 🧩 ✅ No license violations found (64 packages). License distribution: 15× MIT License, 11× Apache Software License, 10× MIT, 7× Apache-2.0, 3× BSD-3-Clause, 2× BSD License, 2× ISC License (ISCL), 2× PSF-2.0, +10 more Full breakdown — 64 packages
Copyright (c) 2022 Phil Ewels Permission is hereby granted, free of charge, to any person obtaining a copy The above copyright notice and this permission notice shall be included in all THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR Copyright (c) 2016 Jannik Michelfeit Permission is hereby granted, free of charge, to any person obtaining a copy The above copyright notice and this permission notice shall be included in all THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 📊 CoverageExploring the coverage frontier of your PR. 🚀 ❌ 21.5% total coverage Per-file coverage (3 files)
Full report: download the 🔒 Security (pip-audit)Ensuring our cookies are secure and fresh. 🍪 ✅ No known vulnerabilities found (84 packages scanned). The inspector has left the building 🕵️ |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
__init__.py (1)
476-479:⚠️ Potential issue | 🟡 MinorPipeline failure: Remove unused variable
e.The
except Exception as e:captures the exception buteis never used. Sinceself.log.exception()automatically includes the exception traceback, the variable is unnecessary.Proposed fix
- except Exception as e: + except Exception: self.log.exception(f"failed to extract date from '{utt}'") dt = now🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 476 - 479, The except block unnecessarily binds the exception to the unused variable "e"; change the handler to not capture the variable by using "except Exception:" (keeping the call to self.log.exception and the fallback assignment dt = now) so the traceback is still logged but no unused variable remains; look for the except clause surrounding the call to extract_datetime (and references to self.log.exception and dt) and update the exception header accordingly.
🧹 Nitpick comments (3)
__init__.py (3)
425-444: Use explicitOptionaltype hints; None guard correctly addresses review feedback.The guard at lines 443-444 properly addresses the PR feedback about potentially passing
Nonetoshow_time. However, update the parameter type hints to use explicitOptional.Proposed fix
- def speak_time(self, dialog: str, location: str = None, - anchor_date: datetime.datetime = None): + def speak_time(self, dialog: str, location: Optional[str] = None, + anchor_date: Optional[datetime.datetime] = None):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 425 - 444, Update the speak_time signature to use explicit Optional type hints: import Optional from typing and change the parameters to location: Optional[str] = None and anchor_date: Optional[datetime.datetime] = None in the speak_time method; keep the existing None-guard logic and show_time call as-is since it already prevents passing None. Refer to the speak_time function name and ensure the typing import is added/updated.
353-367: Use explicitOptionaltype hints per PEP 484.The return type is correctly updated to
Optional[str], but the parameter type hints use implicitOptional(e.g.,location: str = None), which PEP 484 prohibits.Proposed fix
- def get_spoken_time(self, location: str = None, force_ampm=False, - anchor_date: datetime.datetime = None) -> Optional[str]: + def get_spoken_time(self, location: Optional[str] = None, force_ampm: bool = False, + anchor_date: Optional[datetime.datetime] = None) -> Optional[str]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 353 - 367, Update get_spoken_time to use explicit Optional annotations for parameters that default to None: change the signature parameters location and anchor_date to location: Optional[str] = None and anchor_date: Optional[datetime.datetime] = None (keep force_ampm as bool = False). Ensure Optional is imported from typing if not already. This applies to the get_spoken_time method definition so the PEP 484 rule is followed consistently.
379-393: Use explicitOptionaltype hints per PEP 484.Same issue as
get_spoken_time— parameter type hints should use explicitOptional.Proposed fix
- def get_display_time(self, location: str = None, force_ampm=False, - anchor_date: datetime.datetime = None) -> Optional[str]: + def get_display_time(self, location: Optional[str] = None, force_ampm: bool = False, + anchor_date: Optional[datetime.datetime] = None) -> Optional[str]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 379 - 393, The function signature for get_display_time should use explicit Optional annotations like get_spoken_time does: change location: str = None to location: Optional[str] and anchor_date: datetime.datetime = None to anchor_date: Optional[datetime.datetime]; ensure Optional is imported from typing if not already present and leave force_ampm as bool = False. Update the get_display_time declaration accordingly and run type checks to confirm no other changes are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@__init__.py`:
- Around line 476-479: The except block unnecessarily binds the exception to the
unused variable "e"; change the handler to not capture the variable by using
"except Exception:" (keeping the call to self.log.exception and the fallback
assignment dt = now) so the traceback is still logged but no unused variable
remains; look for the except clause surrounding the call to extract_datetime
(and references to self.log.exception and dt) and update the exception header
accordingly.
---
Nitpick comments:
In `@__init__.py`:
- Around line 425-444: Update the speak_time signature to use explicit Optional
type hints: import Optional from typing and change the parameters to location:
Optional[str] = None and anchor_date: Optional[datetime.datetime] = None in the
speak_time method; keep the existing None-guard logic and show_time call as-is
since it already prevents passing None. Refer to the speak_time function name
and ensure the typing import is added/updated.
- Around line 353-367: Update get_spoken_time to use explicit Optional
annotations for parameters that default to None: change the signature parameters
location and anchor_date to location: Optional[str] = None and anchor_date:
Optional[datetime.datetime] = None (keep force_ampm as bool = False). Ensure
Optional is imported from typing if not already. This applies to the
get_spoken_time method definition so the PEP 484 rule is followed consistently.
- Around line 379-393: The function signature for get_display_time should use
explicit Optional annotations like get_spoken_time does: change location: str =
None to location: Optional[str] and anchor_date: datetime.datetime = None to
anchor_date: Optional[datetime.datetime]; ensure Optional is imported from
typing if not already present and leave force_ampm as bool = False. Update the
get_display_time declaration accordingly and run type checks to confirm no other
changes are needed.
3 participants
Summary
__init__.pychanges out of Add missing and incomplete French date-time translations #183Summary by CodeRabbit
New Features
Bug Fixes