Split date-time intent handler code#188
Conversation
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughAdds locale-aware weekday extraction and phrase-stripping, a leap-year query scope classifier, and two public intent handlers ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 |
|
This PR contains only the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
__init__.py (1)
167-250: Avoid baking the locale parsing rules into__init__.py.
_extract_requested_weekday,_strip_requested_weekday_phrase, and_get_leap_year_query_scopeonly understandenandfrright now. That means every new locale for these intents needs a Python change instead of just resource updates, and any other locale will currently fall back toextract.date.error/"current"behavior. Moving these phrase lists into locale resources would keep the new intents aligned with the rest of the skill’s resource-driven translations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 167 - 250, The code hardcodes English/French locale rules in _extract_requested_weekday, _strip_requested_weekday_phrase, and _get_leap_year_query_scope; move those phrase lists and weekday maps into locale resources (e.g. resource keys like "weekdays", "weekday_patterns", "leap_year_phrases" per locale) and change each function to load their locale-specific maps/patterns via the existing resource API (falling back to a safe default or empty dict/list); ensure _extract_requested_weekday still builds the regex-safe names via re.escape on the resource-provided weekday keys and that _strip_requested_weekday_phrase uses the resource phrase as the phrase to remove, and make _get_leap_year_query_scope check resource-provided "either" and "next" phrase lists instead of hardcoded tuples so adding locales requires only resource updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__init__.py`:
- Around line 595-597: The call to self.speak_dialog with both "date" and
"weekday" is executed even when extract_datetime(...) failed, causing
UnboundLocalError because dt and dialog are unset; update the control flow in
the handler so that the self.speak_dialog(... {"date": nice_date(...),
"weekday": nice_weekday(...)}) call is performed only inside the
successful-parse branch (the branch where extract_datetime returns a valid dt
and dialog), and leave the error branch to call the error dialog (e.g.,
"extract.date.error") without referencing dt or nice_weekday; locate
extract_datetime usage and the speak_dialog invocation to adjust the branches
accordingly.
---
Nitpick comments:
In `@__init__.py`:
- Around line 167-250: The code hardcodes English/French locale rules in
_extract_requested_weekday, _strip_requested_weekday_phrase, and
_get_leap_year_query_scope; move those phrase lists and weekday maps into locale
resources (e.g. resource keys like "weekdays", "weekday_patterns",
"leap_year_phrases" per locale) and change each function to load their
locale-specific maps/patterns via the existing resource API (falling back to a
safe default or empty dict/list); ensure _extract_requested_weekday still builds
the regex-safe names via re.escape on the resource-provided weekday keys and
that _strip_requested_weekday_phrase uses the resource phrase as the phrase to
remove, and make _get_leap_year_query_scope check resource-provided "either" and
"next" phrase lists instead of hardcoded tuples so adding locales requires only
resource updates.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
__init__.py (2)
227-250: Avoid hardcoded leap-year scope phrases.
_get_leap_year_query_scopebakes the supported wording into code even though those variants are defined in the locale intent files. If the locale resources change, this helper can silently default back to"current"while the intent still resolves. Prefer separate intents or a structured scope value from the intent parser instead of substring checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 227 - 250, _get_leap_year_query_scope currently hardcodes language-specific phrase lists and uses substring checks, which will break if locale intent resources change; replace this logic by consuming the structured scope value produced by the intent parser (or a dedicated "leap_year_scope" intent slot) instead of inferring from utterance text, i.e., change callers to pass the parsed scope into _get_leap_year_query_scope (or rename it to accept that scope) and remove the language-specific phrase dictionaries and substring checks so the function simply returns/normalizes the intent-provided value ("current"/"next"/"either").
167-225: Move weekday phrase matching out of Python.These helpers duplicate weekday wording that already lives in the locale intent resources, so a translation or phrasing update can keep the intent match working while this post-parse regex starts failing and falls back to
extract.date.error. Prefer passing the matched weekday from the intent layer or vocabulary files instead of reparsing raw utterances here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 167 - 225, The current _extract_requested_weekday re-parses utterances with hard-coded locale regexes; instead remove this internal regex logic and have the date-parsing flow accept the weekday already resolved by the intent/vocabulary layer. Update _extract_requested_weekday to either be removed or changed to a thin adapter that returns the weekday tuple only when a weekday slot/parameter is provided by the caller, and change callers to pass the intent-provided weekday (name or index) into _strip_requested_weekday_phrase; adjust _strip_requested_weekday_phrase to accept the weekday tuple or simple weekday name/index and only perform phrase stripping (no language-specific regex matching of raw utterances). Ensure callers of _extract_requested_weekday and _strip_requested_weekday_phrase (same symbol names) are updated to supply the intent slot value so locale translations live in the intent resources rather than in these functions.
🤖 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 227-250: _get_leap_year_query_scope currently hardcodes
language-specific phrase lists and uses substring checks, which will break if
locale intent resources change; replace this logic by consuming the structured
scope value produced by the intent parser (or a dedicated "leap_year_scope"
intent slot) instead of inferring from utterance text, i.e., change callers to
pass the parsed scope into _get_leap_year_query_scope (or rename it to accept
that scope) and remove the language-specific phrase dictionaries and substring
checks so the function simply returns/normalizes the intent-provided value
("current"/"next"/"either").
- Around line 167-225: The current _extract_requested_weekday re-parses
utterances with hard-coded locale regexes; instead remove this internal regex
logic and have the date-parsing flow accept the weekday already resolved by the
intent/vocabulary layer. Update _extract_requested_weekday to either be removed
or changed to a thin adapter that returns the weekday tuple only when a weekday
slot/parameter is provided by the caller, and change callers to pass the
intent-provided weekday (name or index) into _strip_requested_weekday_phrase;
adjust _strip_requested_weekday_phrase to accept the weekday tuple or simple
weekday name/index and only perform phrase stripping (no language-specific regex
matching of raw utterances). Ensure callers of _extract_requested_weekday and
_strip_requested_weekday_phrase (same symbol names) are updated to supply the
intent slot value so locale translations live in the intent resources rather
than in these functions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__init__.py`:
- Around line 167-187: The cached resources in _load_cached_list and
_load_weekday_match_map are stored under fixed keys and therefore collide across
different locales; update the cache keys to include the instance language
(self.lang) so they are keyed per-language (e.g., use f"{self.lang}.{name}.list"
in _load_cached_list and f"{self.lang}.weekday.match.weekdays.value" in
_load_weekday_match_map), keep the rest of the loading logic the same, and
ensure you reference the new cache_key variable when reading/writing
self.resources.static so each TimeSkill instance caches locale-specific data
separately.
|
@goldyfruit thanks for splitting these out — and I owe you a clearer explanation of what I was actually asking for on #184. My "intents have no handlers" feedback was about the PR being un-mergeable in isolation, not about the file split itself. Splitting #184 into intent-files-only and #188 for handler code addresses the literal text of my comment but creates a new operational problem. The biggest issue is the release workflow. Each merged PR triggers There's also a secondary concern: if #184 lands first, the intents fire with no handlers. If #188 lands first, the handlers try to read The cleaner fix is to recombine #184 and #188 into a single PR that contains the intent files, the locale resources, and the matching handler code in A few thoughts on doing the recombine:
Sorry for the back-and-forth. The split was a reasonable interpretation of what I asked for, but in retrospect I should have said "make this PR self-contained" instead of "the intents have no handlers." Let me know if you'd rather I reopen #184 with the combined work myself, or if you want to drive the recombine. |
|
Superseded by #184. The handler code has been recombined into that branch so the feature can merge as one self-contained alpha update. |
2 participants
Summary
Context
This splits the
__init__.pychanges out of #184 to keep code changes separate from locale changes.Summary by CodeRabbit