feat: use JSON-based euphony rules for word list joining#405
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughExtracted skill helpers into a new util module and switched imports; added many locale Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Join as "join_word_list"
participant CoreRes as "CoreResources"
participant Norm as "_normalize_word"
participant Apply as "_apply_euphony"
Caller->>Join: join_word_list(items, connector, sep, lang)
Join->>CoreRes: load_json_file("word_connectors")
CoreRes-->>Join: connector_word (or fallback)
Join->>CoreRes: load_json_file("euphony")
CoreRes-->>Join: euphony_config (or none)
alt euphony config present
Join->>Norm: _normalize_word(connector_word, normalize_rules)
Norm-->>Join: normalized_connector
Join->>Apply: _apply_euphony(normalized_connector, next_word, rules)
Apply-->>Join: transformed_connector
else no euphony config
Join-->>Join: use connector_word as-is
end
Join->>Join: format list (0,1,2,>=3 items)
Join-->>Caller: formatted_string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
Extracted word joining logic into ovos_workshop/skills/util.py with
generic, language-configurable euphony rules loaded from JSON:
- join_word_list() now supports per-language euphony.json config files
- Italian and Spanish euphony rules defined in locale/{lang}/euphony.json
- Removed hardcoded _join_word_list_it/es() special case handlers
- Rules engine supports: starts_with_vowel, starts_with_letter,
starts_with_any_except conditions with accent normalization
- Moved simple_trace() to util.py for cleaner ovos.py module
This enables language teams to contribute euphony rules without modifying
Python code, improving i18n scalability for ask_selection() formatting.
All existing tests pass; word joining behavior is identical.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Beep! Automated checks have reached 100% completion. 🔋I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthI've checked the repo's balance (aka feature parity). ⚖️ ✅ All required files present. Latest Version: ✅ 🔍 LintA quick update on the status of your PR. 🔔 ❌ ruff: issues found — see job log ⚖️ License CheckKeeping the lawyers happy, one file at a time. 👔 ✅ No license violations found (47 packages). License distribution: 12× MIT License, 8× MIT, 6× Apache Software License, 6× Apache-2.0, 2× BSD-3-Clause, 2× ISC License (ISCL), 2× PSF-2.0, 2× Python Software Foundation License, +7 more Full breakdown — 47 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 Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔒 Security (pip-audit)Checking for any potential security regressions. 🔄 ✅ No known vulnerabilities found (66 packages scanned). 🔨 Build TestsEnsuring all components are in alignment. 📏 ✅ All versions pass
Stay curious and keep coding! 🚀 |
JarbasAl
force-pushed
the
feat/word-join-euphony-json
branch
from
216bcf5 to
5115462
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ovos_workshop/skills/util.py`:
- Line 148: The function signature for join_word_list currently requires sep and
lang causing breakage; restore the default values to match the docstring (make
sep and lang optional with the documented defaults) by changing the
join_word_list signature to provide default values for sep and lang so existing
callers without those args continue to work; update the function declaration
(join_word_list) and any doc references if necessary so the default behavior
documented at lines ~166-168 is reflected in the signature.
- Around line 197-199: The concatenation at the end of the list-joining logic
uses items[-1] without ensuring it's a string, which can raise a TypeError when
the last element isn't str; update the return expression in the function that
builds the joined string to cast the final item to str (e.g., replace items[-1]
with str(items[-1])) so both the earlier generator and the final concatenation
consistently produce strings (referencing the variables items and connector_word
in the existing return expression).
🪄 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: d71991c2-4b4e-4d92-a425-0bedf8ed75f3
📒 Files selected for processing (4)
ovos_workshop/locale/es-ES/euphony.jsonovos_workshop/locale/it-IT/euphony.jsonovos_workshop/skills/ovos.pyovos_workshop/skills/util.py
… tests Added word connectors (and/or translations) for: ar-SA, bg-BG, el-GR, et-EE, eu-ES, fi-FI, he-IL, hi-IN, hr-HR, hu-HU, id-ID, ja-JP, ko-KR, lt-LT, lv-LV, ms-MY, nb-NO, pt-BR, ro-RO, ru-RU, sk-SK, sv-SE, sw-KE, th-TH, tr-TR, vi-VN, zh-CN. Tests added: - simple_trace() formatting - _normalize_word() accent/h-stripping - _apply_euphony() rule engine (all condition types) - euphony.json schema validation for all locales - word_connectors.json presence and schema for all locales - join_word_list() for 9 new languages (ru, tr, ja, zh, ar, sv, hu, pt-BR) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- oc-FR: "e" → "et" before any vowel - ast-ES: "y" → "e" before /i/ (Spanish-like), "o" → "u" before /o/ - an-ES: "y" → "e" before /i/ (Spanish-like), "o" → "u" before /o/ All three are the remaining languages with productive conjunction euphony. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
JarbasAl
changed the title
The old test imported removed internal functions (_join_word_list_it, _join_word_list_es). Updated to use join_word_list from util.py with the lang parameter, preserving all original test cases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/unittests/test_locale_lookup.py (1)
273-283: Validate the condition-specific fields in the schema test.Right now a rule passes with just
connector,condition, andreplace_with, but_apply_euphony()also depends onvowels,letters, and sometimesexcluded_patterns. That lets malformed locale configs pass CI and then silently no-op at runtime.♻️ Tighten the assertions
self.assertIn("rules", data, f"{folder}/euphony.json missing 'rules'") self.assertIsInstance(data["rules"], list, f"{folder}/euphony.json 'rules' must be a list") + required_by_condition = { + "starts_with_vowel": ("vowels",), + "starts_with_letter": ("letters",), + "starts_with_any_except": ("letters", "excluded_patterns"), + } for rule in data["rules"]: self.assertIn("connector", rule, f"{folder}/euphony.json rule missing 'connector'") self.assertIn("condition", rule, f"{folder}/euphony.json rule missing 'condition'") self.assertIn("replace_with", rule, f"{folder}/euphony.json rule missing 'replace_with'") + self.assertIn(rule["condition"], required_by_condition, + f"{folder}/euphony.json has unsupported condition") + for key in required_by_condition[rule["condition"]]: + self.assertIn(key, rule, + f"{folder}/euphony.json rule missing '{key}'")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_locale_lookup.py` around lines 273 - 283, The test currently only asserts presence of "connector", "condition", and "replace_with" but _apply_euphony() also expects rule fields like "vowels", "letters" and sometimes "excluded_patterns", so update the loop in test_locale_lookup.py to assert that each rule includes "vowels" and "letters" and that they are the correct types (e.g., lists or strings as your implementation expects), and if a rule uses excluded_patterns semantics assert "excluded_patterns" exists and is a list (or validate its type/contents). Reference the rule iteration block and the _apply_euphony() behavior when adding these extra assertions so malformed euphony.json rules fail the unit test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ovos_workshop/locale/an-ES/euphony.json`:
- Around line 4-6: The euphony normalization misses accented o, so names like
"Óscar" don't normalize to "o" before _apply_euphony() checks the first
character; add mappings for "ó" -> "o" and "Ó" -> "O" inside the
"replace_accents" object in an-ES/euphony.json (alongside the existing "í":"i")
so the first character is normalized correctly and the o → u rule can match.
In `@test/unittests/test_locale_lookup.py`:
- Around line 271-272: The tests open locale JSON fixtures with open(path) which
relies on system default encoding; change both occurrences (the one that wraps
data = json.load(f) at the two spots) to open the file with explicit UTF-8 by
passing encoding='utf-8' so the Arabic/Cyrillic/CJK characters are read
consistently across platforms; update the two open(path) calls to include
encoding='utf-8' adjacent to the existing arguments.
---
Nitpick comments:
In `@test/unittests/test_locale_lookup.py`:
- Around line 273-283: The test currently only asserts presence of "connector",
"condition", and "replace_with" but _apply_euphony() also expects rule fields
like "vowels", "letters" and sometimes "excluded_patterns", so update the loop
in test_locale_lookup.py to assert that each rule includes "vowels" and
"letters" and that they are the correct types (e.g., lists or strings as your
implementation expects), and if a rule uses excluded_patterns semantics assert
"excluded_patterns" exists and is a list (or validate its type/contents).
Reference the rule iteration block and the _apply_euphony() behavior when adding
these extra assertions so malformed euphony.json rules fail the unit test.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e806a0ad-04b8-4a91-b7a3-c6588db161f4
📒 Files selected for processing (40)
ovos_workshop/locale/an-ES/euphony.jsonovos_workshop/locale/an-ES/word_connectors.jsonovos_workshop/locale/ar-SA/word_connectors.jsonovos_workshop/locale/ast-ES/euphony.jsonovos_workshop/locale/ast-ES/word_connectors.jsonovos_workshop/locale/bg-BG/word_connectors.jsonovos_workshop/locale/cs-CZ/noise_words.listovos_workshop/locale/de-DE/noise_words.listovos_workshop/locale/el-GR/word_connectors.jsonovos_workshop/locale/en-US/noise_words.listovos_workshop/locale/et-EE/word_connectors.jsonovos_workshop/locale/eu-ES/word_connectors.jsonovos_workshop/locale/fi-FI/word_connectors.jsonovos_workshop/locale/fr-FR/noise_words.listovos_workshop/locale/he-IL/word_connectors.jsonovos_workshop/locale/hi-IN/word_connectors.jsonovos_workshop/locale/hr-HR/word_connectors.jsonovos_workshop/locale/hu-HU/word_connectors.jsonovos_workshop/locale/id-ID/word_connectors.jsonovos_workshop/locale/ja-JP/word_connectors.jsonovos_workshop/locale/ko-KR/word_connectors.jsonovos_workshop/locale/lt-LT/word_connectors.jsonovos_workshop/locale/lv-LV/word_connectors.jsonovos_workshop/locale/ms-MY/word_connectors.jsonovos_workshop/locale/nb-NO/word_connectors.jsonovos_workshop/locale/oc-FR/euphony.jsonovos_workshop/locale/oc-FR/word_connectors.jsonovos_workshop/locale/pt-BR/word_connectors.jsonovos_workshop/locale/ro-RO/word_connectors.jsonovos_workshop/locale/ru-RU/noise_words.listovos_workshop/locale/ru-RU/word_connectors.jsonovos_workshop/locale/sk-SK/word_connectors.jsonovos_workshop/locale/sv-SE/word_connectors.jsonovos_workshop/locale/sw-KE/word_connectors.jsonovos_workshop/locale/th-TH/word_connectors.jsonovos_workshop/locale/tr-TR/word_connectors.jsonovos_workshop/locale/vi-VN/word_connectors.jsonovos_workshop/locale/zh-CN/word_connectors.jsontest/unittests/test_euphony.pytest/unittests/test_locale_lookup.py
💤 Files with no reviewable changes (5)
- ovos_workshop/locale/de-DE/noise_words.list
- ovos_workshop/locale/en-US/noise_words.list
- ovos_workshop/locale/fr-FR/noise_words.list
- ovos_workshop/locale/ru-RU/noise_words.list
- ovos_workshop/locale/cs-CZ/noise_words.list
✅ Files skipped from review due to trivial changes (32)
- ovos_workshop/locale/el-GR/word_connectors.json
- ovos_workshop/locale/lt-LT/word_connectors.json
- ovos_workshop/locale/fi-FI/word_connectors.json
- ovos_workshop/locale/ar-SA/word_connectors.json
- ovos_workshop/locale/id-ID/word_connectors.json
- ovos_workshop/locale/et-EE/word_connectors.json
- ovos_workshop/locale/bg-BG/word_connectors.json
- ovos_workshop/locale/hr-HR/word_connectors.json
- ovos_workshop/locale/ms-MY/word_connectors.json
- ovos_workshop/locale/an-ES/word_connectors.json
- ovos_workshop/locale/vi-VN/word_connectors.json
- ovos_workshop/locale/hu-HU/word_connectors.json
- ovos_workshop/locale/hi-IN/word_connectors.json
- ovos_workshop/locale/ast-ES/word_connectors.json
- ovos_workshop/locale/nb-NO/word_connectors.json
- ovos_workshop/locale/he-IL/word_connectors.json
- ovos_workshop/locale/ko-KR/word_connectors.json
- ovos_workshop/locale/pt-BR/word_connectors.json
- ovos_workshop/locale/oc-FR/word_connectors.json
- ovos_workshop/locale/ro-RO/word_connectors.json
- ovos_workshop/locale/lv-LV/word_connectors.json
- ovos_workshop/locale/tr-TR/word_connectors.json
- ovos_workshop/locale/ja-JP/word_connectors.json
- ovos_workshop/locale/sw-KE/word_connectors.json
- ovos_workshop/locale/th-TH/word_connectors.json
- ovos_workshop/locale/sv-SE/word_connectors.json
- ovos_workshop/locale/oc-FR/euphony.json
- ovos_workshop/locale/eu-ES/word_connectors.json
- ovos_workshop/locale/ru-RU/word_connectors.json
- ovos_workshop/locale/sk-SK/word_connectors.json
- ovos_workshop/locale/zh-CN/word_connectors.json
- ovos_workshop/locale/ast-ES/euphony.json
Contributor
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @JarbasAl. The following files were modified: * `ovos_workshop/skills/ovos.py` * `ovos_workshop/skills/util.py` These files were kept as they were: * `test/unittests/test_euphony.py` * `test/unittests/test_locale_lookup.py` These file types are not supported: * `ovos_workshop/locale/an-ES/euphony.json` * `ovos_workshop/locale/an-ES/word_connectors.json` * `ovos_workshop/locale/ar-SA/word_connectors.json` * `ovos_workshop/locale/ast-ES/euphony.json` * `ovos_workshop/locale/ast-ES/word_connectors.json` * `ovos_workshop/locale/bg-BG/word_connectors.json` * `ovos_workshop/locale/el-GR/word_connectors.json` * `ovos_workshop/locale/es-ES/euphony.json` * `ovos_workshop/locale/et-EE/word_connectors.json` * `ovos_workshop/locale/eu-ES/word_connectors.json` * `ovos_workshop/locale/fi-FI/word_connectors.json` * `ovos_workshop/locale/he-IL/word_connectors.json` * `ovos_workshop/locale/hi-IN/word_connectors.json` * `ovos_workshop/locale/hr-HR/word_connectors.json` * `ovos_workshop/locale/hu-HU/word_connectors.json` * `ovos_workshop/locale/id-ID/word_connectors.json` * `ovos_workshop/locale/it-IT/euphony.json` * `ovos_workshop/locale/ja-JP/word_connectors.json` * `ovos_workshop/locale/ko-KR/word_connectors.json` * `ovos_workshop/locale/lt-LT/word_connectors.json` * `ovos_workshop/locale/lv-LV/word_connectors.json` * `ovos_workshop/locale/ms-MY/word_connectors.json` * `ovos_workshop/locale/nb-NO/word_connectors.json` * `ovos_workshop/locale/oc-FR/euphony.json` * `ovos_workshop/locale/oc-FR/word_connectors.json` * `ovos_workshop/locale/pt-BR/word_connectors.json` * `ovos_workshop/locale/ro-RO/word_connectors.json` * `ovos_workshop/locale/ru-RU/word_connectors.json` * `ovos_workshop/locale/sk-SK/word_connectors.json` * `ovos_workshop/locale/sv-SE/word_connectors.json` * `ovos_workshop/locale/sw-KE/word_connectors.json` * `ovos_workshop/locale/th-TH/word_connectors.json` * `ovos_workshop/locale/tr-TR/word_connectors.json` * `ovos_workshop/locale/vi-VN/word_connectors.json` * `ovos_workshop/locale/zh-CN/word_connectors.json`
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
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
1 participant
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
Refactors word list joining to use JSON-based euphony rules and adds i18n support for 46 languages.
JSON-based euphony engine
join_word_list(),simple_trace()and helpers intoovos_workshop/skills/util.pylocale/{lang}/euphony.json) instead of hardcoded Pythonstarts_with_vowel— transform connector before specific vowelsstarts_with_letter— transform connector before specific lettersstarts_with_any_except— transform unless word starts with excluded patternsLanguages with euphony rules (5)
These are the only 5 languages with productive conjunction euphony (confirmed by linguistic research).
Word connectors added (29 new languages)
an-ES, ar-SA, ast-ES, bg-BG, el-GR, et-EE, eu-ES, fi-FI, he-IL, hi-IN, hr-HR, hu-HU, id-ID, ja-JP, ko-KR, lt-LT, lv-LV, ms-MY, nb-NO, oc-FR, pt-BR, ro-RO, ru-RU, sk-SK, sv-SE, sw-KE, th-TH, tr-TR, vi-VN, zh-CN
Code changes
ovos_workshop/skills/util.py— new module withjoin_word_list(),simple_trace(), euphony helpersovos_workshop/skills/ovos.py— imports from util, removed inline implementations_join_word_list_it()and_join_word_list_es()Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Tests