Split leap-year and weekday-match intents#184
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 (43)
✅ Files skipped from review due to trivial changes (39)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR extends TimeSkill with helpers for localized weekday entities and leap-year scope, adds two intent handlers (weekday.matches.date and is.leap.year), and supplies corresponding English and French locale data: intents, dialogs, vocab, and weekday entity files. ChangesTimeSkill weekday & leap-year
English locale resources
French locale resources
Sequence Diagram(s)sequenceDiagram
participant User
participant Intent as Intent Recognizer
participant Handler as handle_weekday_match
participant DateParser as Date Parser
participant Dialog as Dialog System
User->>Intent: "does July 4th fall on a Monday?"
Intent->>Handler: weekday.matches.date.intent (weekday="monday", date="July 4th")
Handler->>Handler: _get_requested_weekday(entities)
Handler->>DateParser: parse date from message
DateParser-->>Handler: datetime dt
Handler->>Handler: compare dt.weekday() with requested weekday index
alt Match Found
Handler->>Dialog: speak weekday.matches.date.{tense}.yes (date, weekday)
else No Match
Handler->>Dialog: speak weekday.matches.date.{tense}.no (date, actual_weekday, weekday)
end
Dialog-->>User: Confirmation or correction
sequenceDiagram
participant User
participant Intent as Intent Recognizer
participant Handler as handle_is_leap_year
participant Classifier as _get_leap_year_query_scope
participant Calendar as calendar.isleap
participant Dialog as Dialog System
User->>Intent: "is next year a leap year?"
Intent->>Handler: is.leap.year.intent
Handler->>Classifier: classify scope from utterance (next/current/either)
Classifier-->>Handler: scope
Handler->>Calendar: calendar.isleap(year)
Calendar-->>Handler: True/False
Handler->>Dialog: speak leap.year.{scope}.{yes|no} with year parameter
Dialog-->>User: Answer about leap year
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
translations/fr-fr/intents.json (1)
20-29: Consider addingl'an prochainvariants here.
_get_leap_year_query_scope()already checks foran prochain, but this intent set only trainsl'année prochaine. Adding one or twol'an prochainexamples would widen French coverage and exercise the branch you just added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@translations/fr-fr/intents.json` around lines 20 - 29, The intent examples for "/intents/is.leap.year.intent" lack colloquial "l'an prochain" variants needed to train the model branch used by _get_leap_year_query_scope(); add one or two phrases such as "l'an prochain est‑ce bissextile" or "l'an prochain est‑elle bissextile" to translations/fr-fr/intents.json under that intent so the "an prochain" pattern is covered.
🤖 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 621-632: The current comparison dt.date() >= now.date() treats
same-day queries as future; change the branching to three-way logic: if
dt.date() > now.date() treat as future (use the existing
"weekday.matches.date.future.yes"/"future.no"), elif dt.date() == now.date()
handle a neutral/today path (add dialog keys like
"weekday.matches.date.today.yes" and "weekday.matches.date.today.no") and else
treat as past (keep "weekday.matches.date.past.*"); update the code around the
dialog variable assignment and use dt.weekday() and expected_weekday exactly as
before to choose yes/no variants.
- Around line 200-202: The French weekday matcher only matches "tombe..." forms
so queries like "tombait-il" or "tombera-t-il" fall through; update the "fr"
regex (the entry containing the phrase pattern
rf"\b(?P<phrase>tombe(?:-t-il)?\s+un\s+(?P<weekday>{names}))\b") to accept
past/future conjugations (e.g., tombait-il, tombera-t-il, tomberait-il) by
expanding the alternation/quantifier in that pattern, so those variants route
into the existing handle_weekday_match() branch; leave handle_weekday_match()
unchanged.
In `@locale/fr-fr/dialog/weekday.matches.date.future.no.dialog`:
- Line 1: The French template string weekday.matches.date.future.no.dialog
currently prefixes the {date} token with a hard-coded "le" which breaks outputs
when nice_date(...) returns standalone relative dates (e.g., "aujourd'hui",
"demain"); update the template for weekday.matches.date.future.no.dialog to
remove the hard-coded "le " and use {date} alone so the generated phrase uses
whatever nice_date(...) returns without an extra article.
In `@locale/fr-fr/dialog/weekday.matches.date.future.yes.dialog`:
- Line 1: The template has a hard-coded "le" before {date} which breaks grammar
when handle_weekday_match() passes a preformatted nice_date(...) like
"aujourd'hui" or "demain"; remove the fixed article so the template reads "Oui,
{date} tombera un {weekday}." and ensure handle_weekday_match() / nice_date()
continues to supply already-correct, localized date strings without adding an
article.
---
Nitpick comments:
In `@translations/fr-fr/intents.json`:
- Around line 20-29: The intent examples for "/intents/is.leap.year.intent" lack
colloquial "l'an prochain" variants needed to train the model branch used by
_get_leap_year_query_scope(); add one or two phrases such as "l'an prochain
est‑ce bissextile" or "l'an prochain est‑elle bissextile" to
translations/fr-fr/intents.json under that intent so the "an prochain" pattern
is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fef5fb79-a973-4c4c-8c21-088ebbd3c38f
📒 Files selected for processing (31)
__init__.pylocale/en-us/dialog/leap.year.current.no.dialoglocale/en-us/dialog/leap.year.current.yes.dialoglocale/en-us/dialog/leap.year.either.no.dialoglocale/en-us/dialog/leap.year.either.yes.dialoglocale/en-us/dialog/leap.year.next.no.dialoglocale/en-us/dialog/leap.year.next.yes.dialoglocale/en-us/dialog/weekday.matches.date.future.no.dialoglocale/en-us/dialog/weekday.matches.date.future.yes.dialoglocale/en-us/dialog/weekday.matches.date.past.no.dialoglocale/en-us/dialog/weekday.matches.date.past.yes.dialoglocale/en-us/intents/is.leap.year.intentlocale/en-us/intents/next.leap.year.intentlocale/en-us/intents/weekday.for.date.intentlocale/en-us/intents/weekday.matches.date.intentlocale/fr-fr/dialog/leap.year.current.no.dialoglocale/fr-fr/dialog/leap.year.current.yes.dialoglocale/fr-fr/dialog/leap.year.either.no.dialoglocale/fr-fr/dialog/leap.year.either.yes.dialoglocale/fr-fr/dialog/leap.year.next.no.dialoglocale/fr-fr/dialog/leap.year.next.yes.dialoglocale/fr-fr/dialog/weekday.matches.date.future.no.dialoglocale/fr-fr/dialog/weekday.matches.date.future.yes.dialoglocale/fr-fr/dialog/weekday.matches.date.past.no.dialoglocale/fr-fr/dialog/weekday.matches.date.past.yes.dialoglocale/fr-fr/intents/is.leap.year.intentlocale/fr-fr/intents/weekday.matches.date.intenttranslations/en-us/dialogs.jsontranslations/en-us/intents.jsontranslations/fr-fr/dialogs.jsontranslations/fr-fr/intents.json
|
Split the |
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)
588-597:⚠️ Potential issue | 🔴 CriticalKeep the weekday response inside the successful parse branch.
Line 595 is currently outside the
else:block, so a failedextract_datetime()still falls through toself.speak_dialog(dialog, ...)withdialogunset.Proposed fix
if not dt: self.speak_dialog("extract.date.error") else: if dt >= now: dialog = "weekday.at.date.future" else: dialog = "weekday.at.date.past" # TODO - "today" should never trigger this intent, but if it does, # should we handle it better? nice_date will return "today" in that case - self.speak_dialog(dialog, { + self.speak_dialog(dialog, { "date": nice_date(dt, lang=self.lang, now=now), "weekday": nice_weekday(dt, lang=self.lang)})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 588 - 597, The speak_dialog call is outside the successful-parse branch causing a failed extract_datetime() to fall through with dialog unset; move the self.speak_dialog(dialog, {...}) call so it is executed inside the else: block where dialog is defined (the branch that sets dialog based on dt >= now) and ensure the same variables (dt, now, self.lang) are in scope for nice_date and nice_weekday; keep extract_datetime usage and the weekday/dialog assignment logic as-is but enclose the speak_dialog invocation within that else branch to prevent speaking with an undefined dialog.
♻️ Duplicate comments (1)
__init__.py (1)
195-199:⚠️ Potential issue | 🟡 MinorSupport
did … fall on …in the English matcher too.Line 197 only matches
does ... fall ..., so past-tense queries like “Did February 29, 2024 fall on a Thursday?” will miss the weekday extraction and end up inextract.date.error.Proposed fix
"en": [ rf"\b(?:is|was)\b.*?\b(?P<phrase>(?:on\s+)?(?:a|an)\s+(?P<weekday>{names}))\b", - rf"\bdoes\b.*?\b(?P<phrase>fall\s+(?:on\s+)?(?:a|an)\s+(?P<weekday>{names}))\b", + rf"\b(?:does|did)\b.*?\b(?P<phrase>fall\s+(?:on\s+)?(?:a|an)\s+(?P<weekday>{names}))\b", rf"\bwill\b.*?\bbe\s+(?P<phrase>(?:on\s+)?(?:a|an)\s+(?P<weekday>{names}))\b", ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 195 - 199, The English regex list for weekday extraction misses past-tense "did ... fall ..." queries (currently only matches "does ... fall ..."), so update the pattern(s) in the "en" array to also match "did" (e.g., change the second pattern that contains \bdoes\b.*?\b(?P<phrase>fall...) to accept \bdoes\b|\bdid\b or add a separate \bdid\b pattern) so past-tense queries like "Did February 29, 2024 fall on a Thursday?" will capture the (?P<phrase>) and (?P<weekday>) groups; adjust the same regex structure used in the existing pattern to keep group names identical.
🧹 Nitpick comments (1)
translations/fr-fr/dialogs.json (1)
98-100: Minor: Inconsistent character encoding style.Line 99 uses the literal character
'étaitwhile other lines in this file (e.g., line 24) use Unicode escapes like\u00e9tait. This is functionally equivalent but creates slight inconsistency. Not a blocker—theprepare_translations.pyandsync_translations.pyscripts will handle either form correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@translations/fr-fr/dialogs.json` around lines 98 - 100, The translation entry for "/dialog/weekday.matches.date.past.no.dialog" uses a literal accented character ("était") which is inconsistent with other entries using Unicode escapes; update the string value to replace "était" with the escaped form ("\u00e9tait") so it matches the file's encoding style and maintain consistency with entries like the one at line 24.
🤖 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 588-597: The speak_dialog call is outside the successful-parse
branch causing a failed extract_datetime() to fall through with dialog unset;
move the self.speak_dialog(dialog, {...}) call so it is executed inside the
else: block where dialog is defined (the branch that sets dialog based on dt >=
now) and ensure the same variables (dt, now, self.lang) are in scope for
nice_date and nice_weekday; keep extract_datetime usage and the weekday/dialog
assignment logic as-is but enclose the speak_dialog invocation within that else
branch to prevent speaking with an undefined dialog.
---
Duplicate comments:
In `@__init__.py`:
- Around line 195-199: The English regex list for weekday extraction misses
past-tense "did ... fall ..." queries (currently only matches "does ... fall
..."), so update the pattern(s) in the "en" array to also match "did" (e.g.,
change the second pattern that contains \bdoes\b.*?\b(?P<phrase>fall...) to
accept \bdoes\b|\bdid\b or add a separate \bdid\b pattern) so past-tense queries
like "Did February 29, 2024 fall on a Thursday?" will capture the (?P<phrase>)
and (?P<weekday>) groups; adjust the same regex structure used in the existing
pattern to keep group names identical.
---
Nitpick comments:
In `@translations/fr-fr/dialogs.json`:
- Around line 98-100: The translation entry for
"/dialog/weekday.matches.date.past.no.dialog" uses a literal accented character
("était") which is inconsistent with other entries using Unicode escapes; update
the string value to replace "était" with the escaped form ("\u00e9tait") so it
matches the file's encoding style and maintain consistency with entries like the
one at line 24.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9e6a373-5af1-4116-856c-d07787566389
📒 Files selected for processing (13)
__init__.pylocale/en-us/dialog/weekday.matches.date.today.no.dialoglocale/en-us/dialog/weekday.matches.date.today.yes.dialoglocale/fr-fr/dialog/weekday.matches.date.future.no.dialoglocale/fr-fr/dialog/weekday.matches.date.future.yes.dialoglocale/fr-fr/dialog/weekday.matches.date.past.no.dialoglocale/fr-fr/dialog/weekday.matches.date.past.yes.dialoglocale/fr-fr/dialog/weekday.matches.date.today.no.dialoglocale/fr-fr/dialog/weekday.matches.date.today.yes.dialoglocale/fr-fr/intents/is.leap.year.intenttranslations/en-us/dialogs.jsontranslations/fr-fr/dialogs.jsontranslations/fr-fr/intents.json
🚧 Files skipped from review as they are similar to previous changes (7)
- translations/en-us/dialogs.json
- locale/fr-fr/dialog/weekday.matches.date.future.yes.dialog
- locale/fr-fr/dialog/weekday.matches.date.past.no.dialog
- translations/fr-fr/intents.json
- locale/fr-fr/dialog/weekday.matches.date.future.no.dialog
- locale/fr-fr/intents/is.leap.year.intent
- locale/fr-fr/dialog/weekday.matches.date.past.yes.dialog
mikejgray
left a comment
mikejgray
left a comment
There was a problem hiding this comment.
Some of these new intents don't have handlers in init.py, which means they would just break where they were moved. However, since you're only updating en-us and fr-fr, a code update must be backwards compatible or you must move the intents across all locales.
It is here #188 |
|
Recombined this PR so it is self-contained again. It now includes:
I am closing #188 as superseded so this can merge as one atomic alpha update. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
__init__.py (1)
573-579:⚠️ Potential issue | 🟡 MinorUse date-only comparison to avoid same-day misclassification in weekday-for-date replies.
At Line 573,
dt >= nowcompares full timestamps. Ifdtis date-only (often midnight), same-day queries can fall into the past branch depending on current time.Suggested fix
- if dt >= now: + if dt.date() > now.date(): dialog = "weekday.at.date.future" + elif dt.date() == now.date(): + dialog = "weekday.at.date.future" # or a dedicated "today" dialog if available else: dialog = "weekday.at.date.past"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 573 - 579, The comparison dt >= now uses full datetimes and misclassifies same-day date-only inputs; update the condition to compare dates only (e.g., compute left = dt.date() if dt has .date() else dt and right = now.date() and then use left >= right) so the intent selection before calling self.speak_dialog(dialog, {...}) uses date-only semantics; modify the branch that sets dialog (currently using dt and now) to use these date-only variables (or normalized datetimes at midnight) to avoid same-day misclassification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@__init__.py`:
- Around line 573-579: The comparison dt >= now uses full datetimes and
misclassifies same-day date-only inputs; update the condition to compare dates
only (e.g., compute left = dt.date() if dt has .date() else dt and right =
now.date() and then use left >= right) so the intent selection before calling
self.speak_dialog(dialog, {...}) uses date-only semantics; modify the branch
that sets dialog (currently using dt and now) to use these date-only variables
(or normalized datetimes at midnight) to avoid same-day misclassification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 306e81c2-85b4-4562-89a5-368bef5edfc0
📒 Files selected for processing (11)
__init__.pylocale/en-us/leap.year.scope.either_phrases.listlocale/en-us/leap.year.scope.next_phrases.listlocale/en-us/weekday.match.patterns.listlocale/en-us/weekday.match.strip_prefixes.listlocale/en-us/weekday.match.strip_suffixes.listlocale/en-us/weekday.match.weekdays.valuelocale/fr-fr/leap.year.scope.either_phrases.listlocale/fr-fr/leap.year.scope.next_phrases.listlocale/fr-fr/weekday.match.patterns.listlocale/fr-fr/weekday.match.weekdays.value
✅ Files skipped from review due to trivial changes (9)
- locale/en-us/leap.year.scope.next_phrases.list
- locale/en-us/leap.year.scope.either_phrases.list
- locale/fr-fr/leap.year.scope.either_phrases.list
- locale/en-us/weekday.match.strip_suffixes.list
- locale/fr-fr/weekday.match.weekdays.value
- locale/fr-fr/leap.year.scope.next_phrases.list
- locale/en-us/weekday.match.weekdays.value
- locale/en-us/weekday.match.strip_prefixes.list
- locale/fr-fr/weekday.match.patterns.list
The bots have finished their work. Take a look! 🤖I've aggregated the results of the automated checks for this PR below. 🎙️ SkillI've given your skill a thorough inspection! 🕵️ 🎙️ ovos-skill-date-time.openvoiceos — 20 languages en-US: 14 intents · 2 vocab · 30 dialogs · skill.json ✅ Translation coverage — 19 languages (1 complete, 7 partial, 11 incomplete)
🔍 LintAnother check completed successfully! 🏁 ✅ ruff: no issues 📊 CoverageA deep dive into the sea of test results. 🌊 ❌ 20.2% total coverage Per-file coverage (3 files)
Full report: download the 🔌 Skill Tests (ovoscope)Scanning for any 'resource leaks' during long usage. 🚰 ✅ 0/0 passed 🚌 Bus CoveragePeeking under the hood at the message traffic. 🏎️ 🏷️ Release PreviewI've checked the 'Legal' section for the release. ⚖️ Current:
🚀 Release Channel Compatibility Predicted next version:
📋 Repo HealthI've checked the repo's flexibility (aka refactorability). 🧘♂️ ✅ All required files present. Latest Version: ✅ 🔒 Security (pip-audit)Evaluating the risk associated with these changes. ⚖️ ✅ No known vulnerabilities found (84 packages scanned). 🔨 Build TestsChecking if the code is ready for prime time. 📺 ✅ All versions pass
⚖️ License CheckI've checked the licenses of our bundled assets. 📦 ✅ 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. Your automated companion in the OpenVoiceOS journey. 🤝 |
| @@ -0,0 +1 @@ | |||
| \b(?P<phrase>(?:tombe(?:-t-il)?|tombait(?:-il)?|tombera(?:-t-il)?|tomberait(?:-il)?)\s+un\s+(?P<weekday>__WEEKDAY_NAMES__))\b | |||
There was a problem hiding this comment.
this is very hard to translate for regular users, please try to avoid regex
|
do we need a extract-weekday in ovos-date-parser? i'd prefer to keep skills minimal and more focused on the voice part. I also discourage using regex for translatable resources. what about using |
|
Addressed Jarbas' review points on this branch.
|
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 670-705: handle_is_leap_year currently passes the non-leap year
into speak_dialog for the "next" and "current" scopes; update the call sites so
when calendar.isleap(next_year) is False you pass {"year":
get_next_leap_year(next_year)} and when calendar.isleap(current_year) is False
you pass {"year": get_next_leap_year(current_year)} (leave the "either" branch
as-is or optionally use get_next_leap_year(next_year) there); modify the
branches around calendar.isleap in handle_is_leap_year to compute and pass the
correct next leap year to speak_dialog.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3046f41f-7d34-422b-8945-84f42b28765f
📒 Files selected for processing (13)
__init__.pylocale/en-US/intents/weekday.matches.date.intentlocale/en-US/vocab/leap.year.scope.either.voclocale/en-US/vocab/leap.year.scope.next.voclocale/en-US/weekday.entitylocale/fr-FR/intents/weekday.matches.date.intentlocale/fr-FR/vocab/leap.year.scope.either.voclocale/fr-FR/vocab/leap.year.scope.next.voclocale/fr-FR/weekday.entitytranslations/en-us/entities.jsontranslations/en-us/intents.jsontranslations/fr-fr/entities.jsontranslations/fr-fr/intents.json
✅ Files skipped from review due to trivial changes (10)
- locale/fr-FR/vocab/leap.year.scope.either.voc
- locale/en-US/vocab/leap.year.scope.either.voc
- locale/fr-FR/vocab/leap.year.scope.next.voc
- locale/fr-FR/intents/weekday.matches.date.intent
- locale/en-US/intents/weekday.matches.date.intent
- locale/fr-FR/weekday.entity
- locale/en-US/weekday.entity
- translations/fr-fr/entities.json
- translations/en-us/entities.json
- locale/en-US/vocab/leap.year.scope.next.voc
🚧 Files skipped from review as they are similar to previous changes (1)
- translations/en-us/intents.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__init__.py (1)
687-689: Consider simplifying toget_next_leap_year(next_year)for consistency.Line 689 uses
get_next_leap_year(next_year + 1), while lines 697 and 705 useget_next_leap_year(next_year)andget_next_leap_year(current_year)respectively. Sinceget_next_leap_year(year)returns the next leap year afteryear, the+ 1is unnecessary whennext_yearis already confirmed non-leap. Usingget_next_leap_year(next_year)would yield the same result and maintain consistency.♻️ Optional consistency fix
else: self.speak_dialog("leap.year.either.no", - {"year": get_next_leap_year(next_year + 1)}) + {"year": get_next_leap_year(next_year)})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 687 - 689, Replace the call get_next_leap_year(next_year + 1) with get_next_leap_year(next_year) to remove the unnecessary +1; locate the branch that calls self.speak_dialog("leap.year.either.no", {"year": ...}) and update the argument so it consistently uses get_next_leap_year(next_year) (the helper get_next_leap_year already returns the next leap year after the given year).
🤖 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 687-689: Replace the call get_next_leap_year(next_year + 1) with
get_next_leap_year(next_year) to remove the unnecessary +1; locate the branch
that calls self.speak_dialog("leap.year.either.no", {"year": ...}) and update
the argument so it consistently uses get_next_leap_year(next_year) (the helper
get_next_leap_year already returns the next leap year after the given year).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dde80695-1bfd-48db-9393-e78e464ca805
📒 Files selected for processing (13)
__init__.pylocale/en-US/intents/weekday.matches.date.intentlocale/en-US/vocab/leap.year.scope.either.voclocale/en-US/vocab/leap.year.scope.next.voclocale/en-US/weekday.entitylocale/fr-FR/intents/weekday.matches.date.intentlocale/fr-FR/vocab/leap.year.scope.either.voclocale/fr-FR/vocab/leap.year.scope.next.voclocale/fr-FR/weekday.entitytranslations/en-us/entities.jsontranslations/en-us/intents.jsontranslations/fr-fr/entities.jsontranslations/fr-fr/intents.json
✅ Files skipped from review due to trivial changes (11)
- locale/en-US/vocab/leap.year.scope.next.voc
- locale/en-US/vocab/leap.year.scope.either.voc
- locale/fr-FR/vocab/leap.year.scope.next.voc
- locale/fr-FR/intents/weekday.matches.date.intent
- locale/fr-FR/vocab/leap.year.scope.either.voc
- locale/en-US/weekday.entity
- locale/fr-FR/weekday.entity
- translations/fr-fr/entities.json
- translations/en-us/entities.json
- locale/en-US/intents/weekday.matches.date.intent
- translations/fr-fr/intents.json
goldyfruit
force-pushed
the
feat/split-date-check-intents
branch
from
a9ed20a to
66ca248
Compare
3 participants
Summary
is.leap.year.intentweekday.matches.date.intentContext
This pulls the new behavior out of #183 so the French locale PR stays translation-focused.
Summary by CodeRabbit
New Features
Localization
Bug Fixes