Skip to content

Add Portuguese support for ago patterns#5

Open
marcelomogami wants to merge 2 commits intoAmato21:masterfrom
marcelomogami:fix/portuguese-ago-patterns
Open

Add Portuguese support for ago patterns#5
marcelomogami wants to merge 2 commits intoAmato21:masterfrom
marcelomogami:fix/portuguese-ago-patterns

Conversation

@marcelomogami
Copy link
Copy Markdown

  • Fix patternToRegex to escape properly (split/escape/join approach)
  • Add singular/plural support for Portuguese units (dias, semanas, meses)
  • Add 'ago' key for prefix patterns (há X dias)
  • Add 'agosuffix' key for suffix patterns (X dias atrás)
  • Add minutesago/hoursago translations for Portuguese
  • Update autocomplete to support both prefix and suffix formats
  • Add ago/agosuffix keys to French, German, Spanish, English

- Fix patternToRegex to escape properly (split/escape/join approach)
- Add singular/plural support for Portuguese units (dias, semanas, meses)
- Add 'ago' key for prefix patterns (há X dias)
- Add 'agosuffix' key for suffix patterns (X dias atrás)
- Add minutesago/hoursago translations for Portuguese
- Update autocomplete to support both prefix and suffix formats
- Add ago/agosuffix keys to French, German, Spanish, English
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements multi-language support for "ago" and "agosuffix" expressions in the date parser and suggestion engine, updating translation files for German, English, Spanish, French, and Portuguese. Key improvements include refactoring the parser to use localized suffix patterns and enhancing the suggestion logic to handle past date prefixes and suffixes. Feedback focuses on refining the regex for unit capturing in the parser to prevent over-matching and correcting a spelling error in the Portuguese translation.

src/parser.ts Outdated
.map(p => p.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'))
.join('|');
const suffixPattern = escapedSuffix.includes('|') ? `(?:${escapedSuffix})` : escapedSuffix;
const agoRegex = new RegExp(`^(\\d+)\\s+(\\w+)\\s+${suffixPattern}$`, 'i');
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The regex (\w+) for capturing the unit in agosuffix expressions is too broad. This could lead to incorrect parsing if a non-time unit word is used (e.g., "2 foo ago" would be parsed as "2 days ago"). It would be more robust to constrain this to known time units, similar to how unitRegexMap is used for prefix patterns.

Suggested change
const agoRegex = new RegExp(`^(\\d+)\\s+(\\w+)\\s+${suffixPattern}$`, 'i');
const agoRegex = new RegExp(`^(\d+)\s+(${Array.from(this.timeUnitMap.keys()).map(k => k.replace(/[.*+?^${}()|[\\]/g, '\\$&')).join('|')})\s+${suffixPattern}$`, 'i');

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in the latest commit. The regex now uses timeUnitMap.keys() to constrain unit matching to known time units.

in: "em|daqui a",
in: "em|daqui a|daqui",
ago: "há",
agosuffix: "atrás|atras",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The translation for agosuffix includes "atras" (without accent). The correct spelling in Portuguese is "atrás". Please correct this to maintain consistency and grammatical accuracy.

Suggested change
agosuffix: "atrás|atras",
agosuffix: "atrás|atrás",

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentionally kept both forms - many Portuguese speakers type without accents on mobile keyboards. Added a comment in the code explaining this.

- Constrain ago suffix regex to known time units from timeUnitMap
- Add comment explaining intentional unaccented 'atras' variant
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant