Add missing and incomplete French date-time translations#183
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExpands French (fr-fr) locale support with new date/time intents, dialogs, and entities. Refactors TimeSkill class to handle location ambiguity, sanitize location inputs, propagate anchor_date parameters, and improve timezone resolution with better null-safety. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@locale/fr-fr/date.entity`:
- Around line 69-71: The date.entity file contains non-concrete expressions ("la
dernière fois qu'on s'est vus", "la prochaine fête") that train the {date} slot
incorrectly; remove these vague phrases from date.entity and either delete them
or move them into a more appropriate entity or free-form utterances (e.g., a
{relative_event} or plain-speech training set) so that only concrete calendar
references remain in date.entity, ensuring your date intents and slot resolution
use only resolvable date expressions.
- Line 29: Replace the English holiday label "Thanksgiving" in the fr-FR date
entity with its French localization by updating the string "Thanksgiving" to
"Action de grâce" (or "Action de grâce (Thanksgiving)" if you want to retain the
English hint); locate the literal "Thanksgiving" in the fr-fr/date.entity file
and change it to the French phrase so the fr-FR locale uses the expected
francophone label.
In `@locale/fr-fr/intents/current_date.intent`:
- Around line 4-5: The utterances in current_date.intent only use the
city-centric preposition "à {location}", which misses non-city forms like "en
France" or "aux États‑Unis"; update current_date.intent (and the other similar
intents referenced) to add idiomatic French variants using the location entity
(location.entity) such as "en {location}", "au {location}", "aux {location}",
"dans {location}" in addition to "à {location}" so non-city locations are
matched naturally.
In `@locale/fr-fr/intents/next.leap.year.intent`:
- Around line 12-14: The boolean questions currently present in
next.leap.year.intent ("cette année est-elle bissextile", "l'année prochaine
est-elle bissextile", "cette année est-elle bissextile ou la prochaine") should
be removed from next.leap.year.intent and placed into a new dedicated intent
(e.g., is.leap.year.intent or leap.year.boolean.intent) that returns a yes/no
response; update the training utterances by moving those exact lines into the
new intent and ensure the new intent's handler/router responds with a boolean
answer instead of the "next leap year" logic used by next.leap.year.intent.
In `@locale/fr-fr/intents/what.day.is.it.intent`:
- Line 3: The intent what.day.is.it.intent currently only includes the variant
"on est le combien à {location}", which misses country/region prepositions
(e.g., "en France", "au Canada", "aux États‑Unis"); update this intent to either
restrict the {location} slot to city-only values or add additional utterances
covering country/region phrasing such as "on est le combien en {location}", "on
est le combien au {location}", "on est le combien aux {location}" (and include
feminine/masculine forms as needed) so the {location} slot matches both city and
country/region uses. Ensure the slot name {location} (or a new
{location_country} if you separate types) is consistent with
locale/fr-fr/location.entity entries and add the new variants to the same
intent.
In `@locale/fr-fr/intents/what.time.will.it.be.intent`:
- Line 2: The training example "dans {offset} heure quelle heure sera-t-il à
{location}" under-represents country/region forms from
locale/fr-fr/location.entity; add alternative utterances using the correct
French prepositions (e.g. "dans {offset} heure quelle heure sera-t-il en
{location}", "dans {offset} heure quelle heure sera-t-il au {location}", "dans
{offset} heure quelle heure sera-t-il aux {location}") and/or split location
into more specific entity types (e.g. location_city vs location_country) so the
intent in locale/fr-fr/intents/what.time.will.it.be.intent matches both cities
("à {location}") and countries/regions ("en/au/aux {location}").
In `@locale/fr-fr/location.entity`:
- Around line 1-83: The locale entity list contains multi-timezone region names
(e.g., Afrique, Europe, Canada, États-Unis, Russie, Australie) which conflict
with the single-string→timezone mapping in __init__.py (the mapping logic around
lines 195-209); either remove these ambiguous entries from
locale/fr-fr/location.entity so only unambiguous place names remain, or add a
disambiguation step in your resolver (called from the same __init__.py mapping
function) that prompts or maps broad region names to a specific
sub-location/timezone before returning a timezone.
In `@translations/fr-fr/dialogs.json`:
- Around line 16-20: The variants for "/dialog/date.relative.future.dialog"
produce incomplete or malformed sentences; update every array element so each is
a full French sentence that includes both placeholders {date} and {num_days}
with the unit "jours" (or appropriate casing/punctuation) and correct word
order/grammar (e.g., "C'est le {date}, dans {num_days} jours.", "Le {date} sera
dans {num_days} jours.", "On sera le {date} dans {num_days} jours.", "{date} —
dans {num_days} jours."). Ensure no element is missing the "jours" unit and fix
the malformed fourth element so it becomes a valid sentence.
- Around line 22-25: The first variant of the
"/dialog/date.relative.past.dialog" entry is missing the time unit after
{num_days}; update that first string so it includes the day unit (use the
singular “jour” to match the singular variant) — ensure you edit the value in
"/dialog/date.relative.past.dialog" that contains "C'\u00e9tait en {date}, il y
a {num_days}" to include the unit following {num_days} (preserve the {date} and
{num_days} placeholders exactly).
In `@translations/fr-fr/intents.json`:
- Around line 7-12: The French training samples for the weekend-range intent
include single-day queries ("quelle est la date du prochain samedi", "quelle est
la date du prochain dimanche", etc.), causing routing to the weekend handler
instead of the specific-day handlers; remove these single-day phrases from the
weekend-range intent samples in translations/fr-fr/intents.json (and the similar
entries at positions 22-25) and add them to the respective single-day intent
samples, and update the intent resolution logic in the weekend-range vs
single-day handlers so that when the utterance explicitly contains "samedi" or
"dimanche" it prefers the single-day intent (i.e., add a pre-check in the
weekend-range handler to route to the single-day handlers when only one day is
mentioned).
- Around line 129-131: The three French utterances ("cette année est-elle
bissextile", "l'année prochaine est-elle bissextile", "cette année est-elle
bissextile ou la prochaine") belong to a boolean/check intent, not the "next
leap year" intent whose handler in __init__.py (the function that always replies
with the next leap year) is being triggered; move these three utterances out of
the "next leap year" intent in translations/fr-fr/intents.json and add them to
or create a dedicated yes/no leap-year intent (handled by a check function such
as is_leap_year or check_leap_year) so the boolean handler can return true/false
for "this year" or "next year" while the existing next_leap_year handler
continues to return the upcoming leap year.
In `@translations/fr-fr/regexes.json`:
- Around line 2-4: The location extractor regex ("location.rx") currently only
matches "\u00e0|dans|pour" and misses common French country prepositions; update
the alternation to include other prepositions like "en", "au", "aux" (and
optionally "du"/"des"/"chez") so phrases like "en France", "au Canada", "aux
États-Unis" are captured; modify the pattern used in "location.rx" to list these
tokens (e.g., "\u00e0|dans|pour|en|au|aux|du|des|chez") while preserving the
capture group (?P<Location>.*) and word-boundary anchors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d5e4222-6588-48f0-ba43-4a91d58a4885
📒 Files selected for processing (21)
locale/fr-fr/date.entitylocale/fr-fr/dialog/day.current.dialoglocale/fr-fr/dialog/next.leap.year.dialoglocale/fr-fr/dialog/weekday.current.dialoglocale/fr-fr/intents/current_date.intentlocale/fr-fr/intents/date.future.weekend.intentlocale/fr-fr/intents/date.last.weekend.intentlocale/fr-fr/intents/next.leap.year.intentlocale/fr-fr/intents/time.until.intentlocale/fr-fr/intents/weekday.for.date.intentlocale/fr-fr/intents/what.day.is.it.intentlocale/fr-fr/intents/what.month.is.it.intentlocale/fr-fr/intents/what.time.is.it.intentlocale/fr-fr/intents/what.time.will.it.be.intentlocale/fr-fr/intents/what.weekday.is.it.intentlocale/fr-fr/intents/what.year.is.it.intentlocale/fr-fr/location.entitytranslations/fr-fr/dialogs.jsontranslations/fr-fr/entities.jsontranslations/fr-fr/intents.jsontranslations/fr-fr/regexes.json
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
locale/fr-fr/date.entity (1)
69-69:⚠️ Potential issue | 🟠 MajorKeep
{date}examples to resolvable calendar references.
la prochaine pleine luneis an astronomical event, not a concrete date expression the skill can reliably resolve through the date handlers. Training the{date}slot on it makes weekday/date intents accept requests they still cannot answer deterministically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/fr-fr/date.entity` at line 69, Remove the astronomical example "la prochaine pleine lune" from the date entity training and replace it with concrete, resolvable calendar references (e.g., "demain", "le 15 juin 2026", "le prochain lundi") so the {date} slot only learns expressions the date handlers can deterministically resolve; update the locale/fr-fr/date.entity entries that seed the {date} slot accordingly to use these calendar-based phrases.
🧹 Nitpick comments (1)
locale/fr-fr/intents/weekday.for.date.intent (1)
13-14: Avoid fixed articles around{date}in these templates.
locale/fr-fr/date.entityalready contains values with their own article/preposition (le 1er avril 2023,ce mois-ci,aujourd'hui, etc.). These two variants create malformed training phrases such asle le 1er avril 2023anddu aujourd'hui, which just adds noise to the French intent model. I’d drop or rewrite them without hard-codedle/du.Suggested cleanup
-le {date} tombe quel jour -quel est le jour de la semaine du {date}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/fr-fr/intents/weekday.for.date.intent` around lines 13 - 14, The two training templates in weekday.for.date.intent include hard-coded articles around the {date} entity which duplicates articles already present in locale/fr-fr/date.entity; remove the fixed articles so the phrases become generic. Edit the strings "le {date} tombe quel jour" and "quel est le jour de la semaine du {date}" to drop the leading/following articles (e.g., "{date} tombe quel jour" and "quel est le jour de la semaine {date}") so the intent uses the raw {date} token and avoids producing "le le ..." or "du aujourd'hui" variants.
🤖 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-185: The helper _extract_requested_weekday currently returns
the first weekday found anywhere in utt; change it to only match a weekday that
appears after the trigger word "tombe" (so we don't accidentally pick a
different weekday elsewhere in the utterance). Locate the weekday_map and
replace the for-loop logic with a search that first finds the "tombe" occurrence
(e.g., using re.search(r"\btombe\b", utt)) and then either (a) restrict the
weekday regex to require "tombe" before it (e.g.,
re.search(rf"\btombe\b.*\b{weekday}\b", utt)) or (b) extract the substring
following "tombe" and search that substring for \b{weekday}\b; return the index
and weekday only when found after "tombe", otherwise return None.
In `@locale/fr-fr/intents/date.future.weekend.intent`:
- Around line 1-8: The intents file
locale/fr-fr/intents/date.future.weekend.intent mixes “ce week-end” (this
weekend) and “prochain week-end” (next weekend) but the handler in __init__.py
(the code that computes a single “this Saturday/this Sunday” range around lines
588–606) always returns one range; split the user phrases into two distinct
intents (e.g., date.this.weekend.intent vs date.next.weekend.intent) or update
the existing intent handler to inspect the phrase for “ce” vs “prochain” and
branch accordingly (compute this-weekend vs next-weekend ranges) so both
phrasings are resolved correctly.
In `@locale/fr-fr/intents/what.time.is.it.intent`:
- Around line 2-78: The location-aware utterances reaching speak_time() can call
get_spoken_time()/get_display_time() immediately after get_datetime(), causing a
crash when timezone resolution returns None; update speak_time() (or the
callsite that handles {location}) to check the timezone/result from
get_datetime() (or the resolver) before calling get_spoken_time() or
get_display_time(), and if the timezone is None, return/speak the existing
"time.tz.not.found" response instead of proceeding to format the time.
In `@locale/fr-fr/location.rx`:
- Line 1: The fallback regex in locale/fr-fr/location.rx is overbroad because
including "en" causes non-location captures to propagate into _extract_location
and then into handle_query_time; either remove "en" from the fallback
alternation (keep only À/au/aux/dans/pour) or add a post-capture validation in
_extract_location (or in handle_query_time) that checks the captured Location
against the canonical locale location list and returns no location if it is not
a known place; reference the regex with the group name (?P<Location>.*), the
extractor _extract_location(), and the consumer handle_query_time() when
implementing the change.
In `@translations/fr-fr/intents.json`:
- Around line 100-185: The parsed future datetime produced in
handle_query_future_time (the dt/future_dt variable parsed at the current parse
block around the previous Lines 419-427) is being discarded and the response is
built using the current time; update handle_query_future_time so the parsed
future datetime (dt/future_dt) is threaded into the response generation path
(where the answer time is formatted and returned) instead of using now(), i.e.,
replace uses of current-time formatting with the parsed dt/future_dt when
building the reply and ensure any timezone/location logic that was applied to
parse dt is preserved when formatting the response.
---
Duplicate comments:
In `@locale/fr-fr/date.entity`:
- Line 69: Remove the astronomical example "la prochaine pleine lune" from the
date entity training and replace it with concrete, resolvable calendar
references (e.g., "demain", "le 15 juin 2026", "le prochain lundi") so the
{date} slot only learns expressions the date handlers can deterministically
resolve; update the locale/fr-fr/date.entity entries that seed the {date} slot
accordingly to use these calendar-based phrases.
---
Nitpick comments:
In `@locale/fr-fr/intents/weekday.for.date.intent`:
- Around line 13-14: The two training templates in weekday.for.date.intent
include hard-coded articles around the {date} entity which duplicates articles
already present in locale/fr-fr/date.entity; remove the fixed articles so the
phrases become generic. Edit the strings "le {date} tombe quel jour" and "quel
est le jour de la semaine du {date}" to drop the leading/following articles
(e.g., "{date} tombe quel jour" and "quel est le jour de la semaine {date}") so
the intent uses the raw {date} token and avoids producing "le le ..." or "du
aujourd'hui" variants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c8c8811-48c7-4bd9-8709-da6e698441fa
📒 Files selected for processing (33)
__init__.pylocale/fr-fr/date.entitylocale/fr-fr/dialog/date.future.weekend.dialoglocale/fr-fr/dialog/date.last.weekend.dialoglocale/fr-fr/dialog/date.relative.future.dialoglocale/fr-fr/dialog/date.relative.past.dialoglocale/fr-fr/dialog/day.current.dialoglocale/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/current_date.intentlocale/fr-fr/intents/date.future.weekend.intentlocale/fr-fr/intents/date.last.weekend.intentlocale/fr-fr/intents/is.leap.year.intentlocale/fr-fr/intents/next.leap.year.intentlocale/fr-fr/intents/weekday.for.date.intentlocale/fr-fr/intents/weekday.matches.date.intentlocale/fr-fr/intents/what.day.is.it.intentlocale/fr-fr/intents/what.time.is.it.intentlocale/fr-fr/intents/what.time.will.it.be.intentlocale/fr-fr/location.entitylocale/fr-fr/location.rxtranslations/fr-fr/dialogs.jsontranslations/fr-fr/entities.jsontranslations/fr-fr/intents.jsontranslations/fr-fr/regexes.json
✅ Files skipped from review due to trivial changes (3)
- locale/fr-fr/dialog/leap.year.next.yes.dialog
- locale/fr-fr/dialog/leap.year.either.no.dialog
- locale/fr-fr/intents/what.day.is.it.intent
🚧 Files skipped from review as they are similar to previous changes (7)
- locale/fr-fr/intents/date.last.weekend.intent
- locale/fr-fr/intents/next.leap.year.intent
- translations/fr-fr/entities.json
- locale/fr-fr/dialog/day.current.dialog
- translations/fr-fr/regexes.json
- locale/fr-fr/location.entity
- locale/fr-fr/intents/current_date.intent
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
locale/fr-fr/intents/date.future.weekend.intent (1)
5-5:⚠️ Potential issue | 🟡 MinorSemantic mismatch: "ce week-end" vs "prochain week-end" still mixed.
Line 5 (
quelles sont les dates de ce week-end) asks about this weekend, while the other 7 lines ask about the next weekend. On Saturday or Sunday, these refer to different date ranges, but the handler computes a single weekend range without distinguishing the wording.Either remove line 5 to keep this intent strictly "future/next weekend", or split into separate intents (
date.this.weekend.intentvsdate.next.weekend.intent) with handler branching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/fr-fr/intents/date.future.weekend.intent` at line 5, The intent entry "quelles sont les dates de ce week-end" mixes "this weekend" with other utterances for "next weekend", causing the single weekend-range handler to be ambiguous; either remove this line from locale/fr-fr/intents/date.future.weekend.intent to keep it strictly "next weekend", or split into two intents (e.g., date.this.weekend.intent and date.next.weekend.intent) and update the weekend-range handler (the function that computes weekend start/end) to branch based on intent name so it returns the correct date range for "this" vs "next" weekend.
🧹 Nitpick comments (2)
locale/fr-fr/dialog/date.relative.future.dialog (1)
3-3: Line 3 phrasing is acceptable; suggested rephrase is an optional stylistic alternative.Both
Nous serons le {date} dans {num_days}.andDans {num_days}, nous serons le {date}.are idiomatic French for spoken output. The original follows the same pattern as Line 1 (event/date description + time indicator) and is not stiff. If you prefer the time-first emphasis for TTS clarity, the suggested reordering is valid, but no change is required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/fr-fr/dialog/date.relative.future.dialog` at line 3, This is a stylistic suggestion for the French TTS string "Nous serons le {date} dans {num_days}."; if you want time-first emphasis for clearer speech, replace that phrase with the alternative "Dans {num_days}, nous serons le {date}." in the locale entry (update the string value in the date.relative.future.dialog entry), otherwise leave it unchanged.locale/fr-fr/intents/what.time.will.it.be.intent (1)
73-84: Minor:si j'attendssection has incomplete coverage compared to other patterns.The
aprèsanddanspatterns cover singular/plural for hours, minutes, and seconds (lines 1-72), butsi j'attendsonly covers plural forms (heures,minutes) and omits:
- Singular forms:
heure,minute- All
seconde/secondesvariantsThis inconsistency may cause some natural utterances to go unrecognized. Consider adding the missing variants for parity, or accept this as intentional scoping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/fr-fr/intents/what.time.will.it.be.intent` around lines 73 - 84, The "si j'attends" intent pattern currently only includes plural forms ("heures", "minutes"); update the "si j'attends" block in the what.time.will.it.be.intent to add the missing singular variants ("heure", "minute") and add all "seconde"/"secondes" variants (with and without location forms like "au {location}", "dans {location}", "à {location}", "en {location}", "aux {location}") so it matches the parity provided by the "après" and "dans" patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@locale/fr-fr/location.rx`:
- Line 1: The current French location regex (\b(à|au|aux|dans|pour)
(?P<Location>.*)) only matches lowercase prepositions; update the pattern used
by _extract_location() to perform a case-insensitive search (e.g., pass the
re.IGNORECASE flag or compile with (?i)) so sentence-initial capitalized forms
like "À Paris..." are matched; adjust the code that calls re.search() in
_extract_location() (or the compiled regex for Location) to use case-insensitive
matching.
---
Duplicate comments:
In `@locale/fr-fr/intents/date.future.weekend.intent`:
- Line 5: The intent entry "quelles sont les dates de ce week-end" mixes "this
weekend" with other utterances for "next weekend", causing the single
weekend-range handler to be ambiguous; either remove this line from
locale/fr-fr/intents/date.future.weekend.intent to keep it strictly "next
weekend", or split into two intents (e.g., date.this.weekend.intent and
date.next.weekend.intent) and update the weekend-range handler (the function
that computes weekend start/end) to branch based on intent name so it returns
the correct date range for "this" vs "next" weekend.
---
Nitpick comments:
In `@locale/fr-fr/dialog/date.relative.future.dialog`:
- Line 3: This is a stylistic suggestion for the French TTS string "Nous serons
le {date} dans {num_days}."; if you want time-first emphasis for clearer speech,
replace that phrase with the alternative "Dans {num_days}, nous serons le
{date}." in the locale entry (update the string value in the
date.relative.future.dialog entry), otherwise leave it unchanged.
In `@locale/fr-fr/intents/what.time.will.it.be.intent`:
- Around line 73-84: The "si j'attends" intent pattern currently only includes
plural forms ("heures", "minutes"); update the "si j'attends" block in the
what.time.will.it.be.intent to add the missing singular variants ("heure",
"minute") and add all "seconde"/"secondes" variants (with and without location
forms like "au {location}", "dans {location}", "à {location}", "en {location}",
"aux {location}") so it matches the parity provided by the "après" and "dans"
patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2041a806-1cc5-40f6-a863-43a1d3bc30fb
📒 Files selected for processing (36)
__init__.pylocale/fr-fr/date.entitylocale/fr-fr/dialog/date.future.weekend.dialoglocale/fr-fr/dialog/date.last.weekend.dialoglocale/fr-fr/dialog/date.relative.future.dialoglocale/fr-fr/dialog/date.relative.past.dialoglocale/fr-fr/dialog/day.current.dialoglocale/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/next.leap.year.dialoglocale/fr-fr/dialog/weekday.current.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/current_date.intentlocale/fr-fr/intents/date.future.weekend.intentlocale/fr-fr/intents/date.last.weekend.intentlocale/fr-fr/intents/is.leap.year.intentlocale/fr-fr/intents/next.leap.year.intentlocale/fr-fr/intents/time.until.intentlocale/fr-fr/intents/weekday.for.date.intentlocale/fr-fr/intents/weekday.matches.date.intentlocale/fr-fr/intents/what.day.is.it.intentlocale/fr-fr/intents/what.month.is.it.intentlocale/fr-fr/intents/what.time.is.it.intentlocale/fr-fr/intents/what.time.will.it.be.intentlocale/fr-fr/intents/what.weekday.is.it.intentlocale/fr-fr/intents/what.year.is.it.intentlocale/fr-fr/location.entitylocale/fr-fr/location.rxtranslations/fr-fr/regexes.json
🚧 Files skipped from review as they are similar to previous changes (23)
- locale/fr-fr/dialog/weekday.matches.date.future.no.dialog
- locale/fr-fr/date.entity
- locale/fr-fr/dialog/weekday.matches.date.past.yes.dialog
- locale/fr-fr/dialog/leap.year.current.no.dialog
- locale/fr-fr/dialog/leap.year.either.yes.dialog
- locale/fr-fr/intents/next.leap.year.intent
- locale/fr-fr/dialog/day.current.dialog
- locale/fr-fr/dialog/weekday.matches.date.future.yes.dialog
- locale/fr-fr/intents/date.last.weekend.intent
- locale/fr-fr/dialog/leap.year.either.no.dialog
- locale/fr-fr/intents/time.until.intent
- locale/fr-fr/intents/what.year.is.it.intent
- locale/fr-fr/dialog/weekday.matches.date.past.no.dialog
- locale/fr-fr/intents/what.weekday.is.it.intent
- locale/fr-fr/intents/what.day.is.it.intent
- locale/fr-fr/dialog/leap.year.next.no.dialog
- locale/fr-fr/intents/is.leap.year.intent
- locale/fr-fr/intents/weekday.for.date.intent
- locale/fr-fr/dialog/date.relative.past.dialog
- locale/fr-fr/dialog/next.leap.year.dialog
- locale/fr-fr/intents/current_date.intent
- locale/fr-fr/dialog/date.last.weekend.dialog
- locale/fr-fr/intents/what.month.is.it.intent
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
__init__.py (2)
426-444:⚠️ Potential issue | 🟡 MinorPropagate
anchor_dateinto the GUI time page too.The spoken answer now uses
anchor_date, but the time page still renders its date viaget_display_date()with no reference date. Future-time queries will therefore show the requested time alongside today’s date.Proposed fix
- def speak_time(self, dialog: str, location: str = None, - anchor_date: datetime.datetime = None): + def speak_time(self, dialog: str, location: str = None, + anchor_date: datetime.datetime = None): @@ - self.show_time(time_string) + self.show_time(time_string, anchor_date=anchor_date)- def show_time(self, display_time: str): + def show_time(self, display_time: str, + anchor_date: datetime.datetime = None): """Display time on GUI and Mark 1 faceplate.""" - self.show_time_gui(display_time) + self.show_time_gui(display_time, anchor_date=anchor_date) self.show_time_mark1(display_time) - def show_time_gui(self, display_time): + def show_time_gui(self, display_time, + anchor_date: datetime.datetime = None): """ Display time on the GUI. """ self.gui.clear() self.gui['time_string'] = display_time self.gui['ampm_string'] = '' - self.gui['date_string'] = self.get_display_date() + self.gui['date_string'] = self.get_display_date(anchor_date=anchor_date) self.gui.show_page('time')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 426 - 444, The GUI time page isn't using the anchor_date used for speech; update speak_time so both calls to get_display_time include the anchor_date argument (i.e., call get_display_time(location, anchor_date=anchor_date) and get_display_time(anchor_date=anchor_date)), ensuring the displayed time/date matches the spoken future-time; keep existing logic for current_time/speak_dialog and show_time but pass the anchor_date through to the display helper(s).
348-349:⚠️ Potential issue | 🟠 MajorHandle naive
anchor_datebefore callingastimezone().If a caller passes a naive
datetimetoget_datetime(), line 349 will treat it as system local time rather thanself.location_timezone. This causes incorrect timezone conversions when device timezone and session timezone differ. The parameter acceptsdatetime.datetimewith no timezone enforcement, and the docstring does not require it to be timezone-aware.Proposed fix
if anchor_date: - dt = anchor_date.astimezone(tz) + if anchor_date.tzinfo is None: + source_tz = pytz.timezone(self.location_timezone) + anchor_date = source_tz.localize(anchor_date) + dt = anchor_date.astimezone(tz) else: dt = now_local(tz)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 348 - 349, The call to anchor_date.astimezone(tz) assumes anchor_date is timezone-aware; update get_datetime to first detect a naive anchor_date (anchor_date.tzinfo is None) and localize/assign it to self.location_timezone before calling astimezone(tz). Concretely, in the get_datetime flow around the anchor_date/astimezone call, if anchor_date.tzinfo is None attach the session/device timezone (self.location_timezone) to the datetime using the appropriate method for the project's tz objects (pytz.localize or zoneinfo/replace), then proceed to dt = anchor_date.astimezone(tz) so conversions use the intended timezone. Ensure you reference anchor_date, self.location_timezone, tz, and get_datetime when making the change.
🧹 Nitpick comments (1)
locale/fr-fr/location.entity (1)
29-29: Add the article-less alias forau Caire.With
locale/fr-fr/location.rxnow matchingau, the fallback path extractsCaire, while this entity list only containsLe Caire. That leaves the entity and regex paths inconsistent for a common phrasing. AddingCairehere would make recognition more reliable.♻️ Suggested addition
Kenya +Caire Le Caire Lisbonne🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/fr-fr/location.entity` at line 29, Add the article-less alias "Caire" to the locale/fr-fr/location.entity entries so it matches the favicon fallback from locale/fr-fr/location.rx that captures "au Caire" -> "Caire"; update the list that currently contains "Le Caire" to also include "Caire" (ensure the exact token "Caire" is present alongside "Le Caire" in the location.entity collection).
🤖 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 583-585: The fallback input to extract_datetime should exclude the
weekday phrase to avoid biasing parsing: use the previously extracted weekday
(weekday = self._extract_requested_weekday(utterance)) to strip or remove that
weekday substring from utterance before calling extract_datetime, and then call
extract_datetime with the cleaned text (instead of the raw utterance) when
message.data.get("date") is missing; update the call site that currently uses
extract_datetime(message.data.get("date") or utterance, anchorDate=now,
lang=self.lang) so it passes the cleaned utterance while keeping the same
anchorDate/lang parameters and preserving the existing dt, _ assignment logic.
In `@locale/fr-fr/location.rx`:
- Line 1: The fallback regex in locale/fr-fr/location.rx uses a greedy group
(?P<Location>.*) which captures the rest of the utterance and breaks
_extract_location() results; tighten the capture to only the location span or
validate/truncate the captured value before use. Update the regex (the pattern
containing (?P<Location>.*)) to limit characters (e.g., non-greedy or character
class) or, after matching, run the captured Location through the
locale/fr-fr/location.entity lookup/validation and trim any trailing text or
punctuation before passing it to _extract_location() so timezone resolution uses
the true location string.
---
Outside diff comments:
In `@__init__.py`:
- Around line 426-444: The GUI time page isn't using the anchor_date used for
speech; update speak_time so both calls to get_display_time include the
anchor_date argument (i.e., call get_display_time(location,
anchor_date=anchor_date) and get_display_time(anchor_date=anchor_date)),
ensuring the displayed time/date matches the spoken future-time; keep existing
logic for current_time/speak_dialog and show_time but pass the anchor_date
through to the display helper(s).
- Around line 348-349: The call to anchor_date.astimezone(tz) assumes
anchor_date is timezone-aware; update get_datetime to first detect a naive
anchor_date (anchor_date.tzinfo is None) and localize/assign it to
self.location_timezone before calling astimezone(tz). Concretely, in the
get_datetime flow around the anchor_date/astimezone call, if anchor_date.tzinfo
is None attach the session/device timezone (self.location_timezone) to the
datetime using the appropriate method for the project's tz objects
(pytz.localize or zoneinfo/replace), then proceed to dt =
anchor_date.astimezone(tz) so conversions use the intended timezone. Ensure you
reference anchor_date, self.location_timezone, tz, and get_datetime when making
the change.
---
Nitpick comments:
In `@locale/fr-fr/location.entity`:
- Line 29: Add the article-less alias "Caire" to the
locale/fr-fr/location.entity entries so it matches the favicon fallback from
locale/fr-fr/location.rx that captures "au Caire" -> "Caire"; update the list
that currently contains "Le Caire" to also include "Caire" (ensure the exact
token "Caire" is present alongside "Le Caire" in the location.entity
collection).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec5877b4-e987-4aee-9dae-dbbd6e5e6ff1
📒 Files selected for processing (5)
__init__.pylocale/fr-fr/location.entitylocale/fr-fr/location.rxtranslations/fr-fr/entities.jsontranslations/fr-fr/regexes.json
🚧 Files skipped from review as they are similar to previous changes (2)
- translations/fr-fr/entities.json
- translations/fr-fr/regexes.json
Collaborator
Author
|
Split the new behavior into #184 so this PR stays focused on French locale coverage. The leap-year yes/no intent and weekday-match intent now live there with matching English and French resources. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
__init__.py (2)
59-85: Consider usingfrozensetfor immutable class-level lookup tables.Static analysis flags these as mutable class attributes (RUF012). While they're used as read-only lookups, using
frozensetmakes the immutability explicit and prevents accidental modification.♻️ Proposed fix
- AMBIGUOUS_LOCATIONS = { + AMBIGUOUS_LOCATIONS = frozenset({ "australie", "bresil", ... "les états-unis d'amérique" - } - NON_LOCATION_PHRASES = { + }) + NON_LOCATION_PHRASES = frozenset({ "ce moment", "ce moment-ci", "ce moment la", "ce moment-là" - } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 59 - 85, AMBIQUOUS_LOCATIONS and NON_LOCATION_PHRASES are defined as mutable set literals at class/module level; replace their definitions with frozenset(...) to make them immutable and satisfy static analysis (RUF012). Update the declarations for AMBIGUOUS_LOCATIONS and NON_LOCATION_PHRASES to construct frozenset from the current iterable values (e.g., AMBIGUOUS_LOCATIONS = frozenset({...})) so all existing membership checks continue to work while preventing accidental modification.
344-345: Update type hints to use explicitOptionalorT | None.Static analysis (RUF013) flags implicit Optional in function signatures. Parameters with default
Noneshould have their types explicitly annotated asOptional[T]orT | Noneper PEP 484.♻️ Proposed fix (example for get_spoken_time)
- 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]:Also applies to: 370-371, 416-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 344 - 345, The function signature for get_spoken_time uses implicit Optionals; change parameters with default None to explicit optionals (e.g., location: Optional[str] = None and anchor_date: Optional[datetime.datetime] = None) and ensure typing.Optional (or PEP 604 union T | None) is imported/used consistently; apply the same explicit-Optional fix to the other functions in this module that declare parameters with default None so all such params are annotated as Optional[T].locale/fr-fr/location.rx (1)
1-1: Re-add theenpreposition to capture location queries like "quelle heure sera-t-il en France."The pattern intentionally excludes
ento avoid false positives like "en ce moment" → "ce moment". However,_sanitize_location()already filtersNON_LOCATION_PHRASESwhich includes "ce moment", "ce moment-ci", "ce moment la", and "ce moment-là"—the exact false positives that would occur withen.The intent files show "en" is used extensively in core time-query phrases ("quelle heure sera-t-il en {location}"), appearing 12+ times across intent definitions. Other French-equivalent locales (Spanish, Portuguese, Catalan) include their equivalent prepositions, making French the unique exception. Re-adding
enwould not re-introduce false positives because the sanitization filter already handles them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/fr-fr/location.rx` at line 1, The current regex in location.rx excludes the preposition "en", causing missed location captures; update the regex pattern \b(à|au|aux|dans|pour)\s+(?P<Location>[^,;:.!?]+?)(?=\s*[,;:.!?]|$) to re-include "en" so phrases like "quelle heure sera-t-il en France" are matched, and rely on the existing _sanitize_location() and NON_LOCATION_PHRASES filtering to remove false positives (e.g., "ce moment"); ensure the change targets the same pattern token (the preposition group) so other behavior remains unchanged.
🤖 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 59-85: AMBIQUOUS_LOCATIONS and NON_LOCATION_PHRASES are defined as
mutable set literals at class/module level; replace their definitions with
frozenset(...) to make them immutable and satisfy static analysis (RUF012).
Update the declarations for AMBIGUOUS_LOCATIONS and NON_LOCATION_PHRASES to
construct frozenset from the current iterable values (e.g., AMBIGUOUS_LOCATIONS
= frozenset({...})) so all existing membership checks continue to work while
preventing accidental modification.
- Around line 344-345: The function signature for get_spoken_time uses implicit
Optionals; change parameters with default None to explicit optionals (e.g.,
location: Optional[str] = None and anchor_date: Optional[datetime.datetime] =
None) and ensure typing.Optional (or PEP 604 union T | None) is imported/used
consistently; apply the same explicit-Optional fix to the other functions in
this module that declare parameters with default None so all such params are
annotated as Optional[T].
In `@locale/fr-fr/location.rx`:
- Line 1: The current regex in location.rx excludes the preposition "en",
causing missed location captures; update the regex pattern
\b(à|au|aux|dans|pour)\s+(?P<Location>[^,;:.!?]+?)(?=\s*[,;:.!?]|$) to
re-include "en" so phrases like "quelle heure sera-t-il en France" are matched,
and rely on the existing _sanitize_location() and NON_LOCATION_PHRASES filtering
to remove false positives (e.g., "ce moment"); ensure the change targets the
same pattern token (the preposition group) so other behavior remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52359276-6622-422c-8192-7d678d22409f
📒 Files selected for processing (8)
__init__.pylocale/fr-fr/dialog/date.relative.future.dialoglocale/fr-fr/dialog/date.relative.past.dialoglocale/fr-fr/intents/time.until.intentlocale/fr-fr/location.rxtranslations/fr-fr/dialogs.jsontranslations/fr-fr/intents.jsontranslations/fr-fr/regexes.json
🚧 Files skipped from review as they are similar to previous changes (3)
- locale/fr-fr/dialog/date.relative.future.dialog
- locale/fr-fr/intents/time.until.intent
- locale/fr-fr/dialog/date.relative.past.dialog
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Improvements