fix:upper case lang codes #76
Conversation
WalkthroughThe changes made in the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
lingua_franca/parse.py (1)
Line range hint
1-324: Consider applying similar changes to other functions for consistency.While the current change effectively addresses the issue for the
yes_or_nofunction, it might be beneficial to review other functions in this file that accept alangparameter. For consistency and to fully resolve the language code formatting issue across the codebase, consider applying thelang = get_full_lang_code(lang)line at the beginning of other relevant functions that don't currently use it.This would ensure that all functions in this file handle language codes consistently and support standardized (uppercase) codes as intended by this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- lingua_franca/internal.py (3 hunks)
- lingua_franca/parse.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
lingua_franca/parse.py (1)
122-122: LGTM! This change addresses the language code formatting issue.The addition of
lang = get_full_lang_code(lang)at the beginning of theyes_or_nofunction aligns perfectly with the PR objectives. This modification ensures that standardized language codes (including uppercase codes) are properly supported, resolving the issue described in #75. The change is minimal and focused, maintaining backwards compatibility while improving the consistency of language code handling across the codebase.lingua_franca/internal.py (2)
363-363: Enforce consistent casing for language codesConverting the returned
langto lowercase inget_full_lang_codeensures that language codes are consistently handled in lowercase throughout the codebase, which aligns with standard conventions and improves consistency.
500-500: Handle uppercase language codes by normalizing caseLowercasing
lang_codeat line 500 ensures that standardized uppercase language codes are properly handled. This adjustment allows the system to accept language codes in any case, enhancing robustness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
lingua_franca/config.py (1)
1-3: Consider documenting configuration impacts.Since this is a significant behavioral change (lazy loading vs. preloading), it would be helpful to add comments explaining:
- The purpose of lazy loading
- Performance implications
- Any initialization requirements
-load_langs_on_demand = True +# Enable lazy loading of language modules to reduce initial memory footprint +# Languages will be loaded when first accessed +load_langs_on_demand = True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- lingua_franca/config.py (1 hunks)
- lingua_franca/internal.py (2 hunks)
🔇 Additional comments (2)
lingua_franca/config.py (1)
1-1: Verify relevance to uppercase language code support.This change enables on-demand language loading, but it's unclear how this addresses the PR's objective of supporting uppercase language codes (issue #75). Could you explain the relationship between lazy loading and the language code case handling?
Let's verify the impact of this change on language code handling:
lingua_franca/internal.py (1)
363-363:⚠️ Potential issuePotential regression: Forcing lowercase contradicts PR objectives
The change to force language codes to lowercase (
return lang.lower()) appears to contradict the PR objectives of fixing uppercase language code support. This modification could break compatibility with systems expecting standardized uppercase language codes as mentioned in issue #75.Consider preserving the case of the language code:
- return lang.lower() + return langLet's verify the language code handling in the codebase:
|
this PR also fixes that if I ask "Welcher Tag ist heute?" ("what's the date" in german), that ovos responded with the english date instead of the german one, now it responds in german, as it should +1 |
is this still an issue? skills are moving to https://github.com/OpenVoiceOS/ovos-date-parser and dropping lingua-franca assuming same as OpenVoiceOS/ovos-skill-date-time#70 |
|
@JarbasAl updated the date time skill to the newest version, seems to be working :) |
closes #75
see OpenVoiceOS/OVOS-workshop#256
Summary by CodeRabbit
New Features
Bug Fixes
Nonevalues for language codes.Documentation