- language codes and name as dict #1614
Conversation
WalkthroughThe changes involve modifications to the Changes
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: 3
🧹 Outside diff range and nitpick comments (2)
tests/integration_test/services_test.py (2)
28116-28127: Consider using test fixtures for language data.The mock implementation correctly reflects the new dictionary format mapping ISO language codes to their names. However, consider moving this test data to a fixture file for better maintainability and reusability.
You could create a fixture file like
tests/testing_data/language_fixtures.py:SUPPORTED_LANGUAGES_FIXTURE = { "ab": "Abkhaz", "ace": "Acehnese", # ... rest of the languages }Then import and use it in the test:
from tests.testing_data.language_fixtures import SUPPORTED_LANGUAGES_FIXTURE def _mock_supported_languages(*args, **kwargs): return SUPPORTED_LANGUAGES_FIXTURE
28138-28149: Consider reducing data duplication in assertions.The assertion correctly validates the new dictionary format, but duplicating the entire dictionary makes the test harder to maintain. Consider reusing the mock data reference.
If you implement the fixture suggestion above, you could simplify this to:
assert response["data"] == SUPPORTED_LANGUAGES_FIXTUREAlso, consider adding negative test cases to verify error handling:
- Invalid language codes
- Unsupported display languages
- Service unavailability scenarios
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
kairon/shared/multilingual/utils/translator.py(1 hunks)tests/integration_test/services_test.py(2 hunks)
🔇 Additional comments (1)
kairon/shared/multilingual/utils/translator.py (1)
93-93: Verify the impact of hardcoded display language
The hardcoded "en" language code might not be suitable for all use cases.
Let's check if there are any UI components or tests that might be affected:
✅ Verification successful
Let's gather more context about how the get_supported_languages is used in the codebase.
Based on the gathered information, I can now provide the final response:
Hardcoded display language code "en" is appropriate in this context
The hardcoded "en" display language code is actually the correct implementation here because:
- The method returns a mapping of language codes to their English display names (
result = {language.language_code: language.display_name}) - This is used as a system-level API to get supported languages, not for end-user display
- The API endpoint
/api/user/multilingual/languagesconsumes this method to provide a consistent language list interface
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to get_supported_languages
rg -l "get_supported_languages"
# Search for language code references in UI components
rg -l "language.*code|display.*language"
Length of output: 1117
Script:
#!/bin/bash
# Check the implementation and usage of get_supported_languages
rg -A 5 "get_supported_languages" tests/unit_test/multilingual/multilingual_processor_test.py tests/integration_test/services_test.py kairon/api/app/routers/user.py
# Check if there are any language code configurations or settings
rg "display_language_code" -A 2
Length of output: 2913
| response = client.get_supported_languages(parent=parent, display_language_code="en") | ||
| result = {language.language_code: language.display_name for language in response.languages} | ||
| return result |
There was a problem hiding this comment.
Add error handling and make the language code configurable
The API call lacks error handling and uses a hardcoded language code.
Consider this safer implementation:
- response = client.get_supported_languages(parent=parent, display_language_code="en")
- result = {language.language_code: language.display_name for language in response.languages}
- return result
+ try:
+ response = client.get_supported_languages(
+ parent=parent,
+ display_language_code=display_language_code
+ )
+ return {
+ language.language_code: language.display_name
+ for language in response.languages
+ }
+ except Exception as e:
+ logger.exception(e)
+ raise AppException(f'Failed to fetch supported languages: {str(e)}')Committable suggestion skipped: line range outside the PR's diff.
| multilingual_env = Utility.environment.get('multilingual', {}) | ||
| service_account_creds = multilingual_env.get('service_account_creds', {}) | ||
|
|
||
| service_account_info_json = { | ||
| "type": Utility.environment['multilingual']['service_account_creds'].get('type', "service_account"), | ||
| "project_id": Utility.environment['multilingual']['project_id'], | ||
| "private_key_id": Utility.environment['multilingual']['service_account_creds']['private_key_id'], | ||
| "private_key": Utility.environment['multilingual']['service_account_creds']['private_key'], | ||
| "client_email": Utility.environment['multilingual']['service_account_creds']['client_email'], | ||
| "client_id": Utility.environment['multilingual']['service_account_creds']['client_id'], | ||
| "auth_uri": Utility.environment['multilingual']['service_account_creds']['auth_uri'], | ||
| "token_uri": Utility.environment['multilingual']['service_account_creds']['token_uri'], | ||
| "auth_provider_x509_cert_url": Utility.environment['multilingual']['service_account_creds']['auth_provider_x509_cert_url'], | ||
| "client_x509_cert_url": Utility.environment['multilingual']['service_account_creds']['client_x509_cert_url'] | ||
| "type": service_account_creds.get('type', "service_account"), | ||
| "project_id": multilingual_env.get('project_id'), | ||
| "private_key_id": service_account_creds.get('private_key_id'), | ||
| "private_key": service_account_creds.get('private_key'), | ||
| "client_email": service_account_creds.get('client_email'), | ||
| "client_id": service_account_creds.get('client_id'), | ||
| "auth_uri": service_account_creds.get('auth_uri'), | ||
| "token_uri": service_account_creds.get('token_uri'), | ||
| "auth_provider_x509_cert_url": service_account_creds.get('auth_provider_x509_cert_url'), | ||
| "client_x509_cert_url": service_account_creds.get('client_x509_cert_url') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for missing configuration
While the use of .get() improves safety, there are still potential issues:
- Line 77: Direct access to
multilingual_env.get('project_id')could return None - No validation of required credentials before attempting to use them
Consider this safer approach:
multilingual_env = Utility.environment.get('multilingual', {})
service_account_creds = multilingual_env.get('service_account_creds', {})
+
+project_id = multilingual_env.get('project_id')
+if not project_id:
+ raise AppException("Missing required configuration: multilingual.project_id")
+
+required_creds = ['private_key', 'client_email']
+missing_creds = [cred for cred in required_creds if not service_account_creds.get(cred)]
+if missing_creds:
+ raise AppException(f"Missing required credentials: {', '.join(missing_creds)}")
service_account_info_json = {
"type": service_account_creds.get('type', "service_account"),
- "project_id": multilingual_env.get('project_id'),
+ "project_id": project_id,Committable suggestion skipped: line range outside the PR's diff.
| multilingual_env = Utility.environment.get('multilingual', {}) | ||
| service_account_creds = multilingual_env.get('service_account_creds', {}) | ||
|
|
||
| service_account_info_json = { | ||
| "type": Utility.environment['multilingual']['service_account_creds'].get('type', "service_account"), | ||
| "project_id": Utility.environment['multilingual']['project_id'], | ||
| "private_key_id": Utility.environment['multilingual']['service_account_creds']['private_key_id'], | ||
| "private_key": Utility.environment['multilingual']['service_account_creds']['private_key'], | ||
| "client_email": Utility.environment['multilingual']['service_account_creds']['client_email'], | ||
| "client_id": Utility.environment['multilingual']['service_account_creds']['client_id'], | ||
| "auth_uri": Utility.environment['multilingual']['service_account_creds']['auth_uri'], | ||
| "token_uri": Utility.environment['multilingual']['service_account_creds']['token_uri'], | ||
| "auth_provider_x509_cert_url": Utility.environment['multilingual']['service_account_creds']['auth_provider_x509_cert_url'], | ||
| "client_x509_cert_url": Utility.environment['multilingual']['service_account_creds']['client_x509_cert_url'] | ||
| "type": service_account_creds.get('type', "service_account"), | ||
| "project_id": multilingual_env.get('project_id'), | ||
| "private_key_id": service_account_creds.get('private_key_id'), | ||
| "private_key": service_account_creds.get('private_key'), | ||
| "client_email": service_account_creds.get('client_email'), | ||
| "client_id": service_account_creds.get('client_id'), | ||
| "auth_uri": service_account_creds.get('auth_uri'), | ||
| "token_uri": service_account_creds.get('token_uri'), | ||
| "auth_provider_x509_cert_url": service_account_creds.get('auth_provider_x509_cert_url'), | ||
| "client_x509_cert_url": service_account_creds.get('client_x509_cert_url') | ||
| } | ||
| logger.debug(service_account_info_json) | ||
| credentials = service_account.Credentials.from_service_account_info(service_account_info_json) | ||
| client = translate.TranslationServiceClient(credentials=credentials) | ||
|
|
||
| location = "global" | ||
| parent = f"projects/{Utility.environment['multilingual']['project_id']}/locations/{location}" | ||
| response = client.get_supported_languages(parent=parent) | ||
| response = client.get_supported_languages(parent=parent, display_language_code="en") | ||
| result = {language.language_code: language.display_name for language in response.languages} | ||
| return result |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update method signature and documentation
The method signature doesn't match the changes, and documentation is missing for the new return format.
Add the following documentation and signature:
- def get_supported_languages():
+ @staticmethod
+ def get_supported_languages(display_language_code: Text = "en") -> dict:
+ """
+ Get supported languages with their display names.
+
+ Args:
+ display_language_code: Language code for the display names (default: "en")
+
+ Returns:
+ dict: Mapping of language codes to their display names
+
+ Raises:
+ AppException: If the API call fails or configuration is invalid
+ """📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| multilingual_env = Utility.environment.get('multilingual', {}) | |
| service_account_creds = multilingual_env.get('service_account_creds', {}) | |
| service_account_info_json = { | |
| "type": Utility.environment['multilingual']['service_account_creds'].get('type', "service_account"), | |
| "project_id": Utility.environment['multilingual']['project_id'], | |
| "private_key_id": Utility.environment['multilingual']['service_account_creds']['private_key_id'], | |
| "private_key": Utility.environment['multilingual']['service_account_creds']['private_key'], | |
| "client_email": Utility.environment['multilingual']['service_account_creds']['client_email'], | |
| "client_id": Utility.environment['multilingual']['service_account_creds']['client_id'], | |
| "auth_uri": Utility.environment['multilingual']['service_account_creds']['auth_uri'], | |
| "token_uri": Utility.environment['multilingual']['service_account_creds']['token_uri'], | |
| "auth_provider_x509_cert_url": Utility.environment['multilingual']['service_account_creds']['auth_provider_x509_cert_url'], | |
| "client_x509_cert_url": Utility.environment['multilingual']['service_account_creds']['client_x509_cert_url'] | |
| "type": service_account_creds.get('type', "service_account"), | |
| "project_id": multilingual_env.get('project_id'), | |
| "private_key_id": service_account_creds.get('private_key_id'), | |
| "private_key": service_account_creds.get('private_key'), | |
| "client_email": service_account_creds.get('client_email'), | |
| "client_id": service_account_creds.get('client_id'), | |
| "auth_uri": service_account_creds.get('auth_uri'), | |
| "token_uri": service_account_creds.get('token_uri'), | |
| "auth_provider_x509_cert_url": service_account_creds.get('auth_provider_x509_cert_url'), | |
| "client_x509_cert_url": service_account_creds.get('client_x509_cert_url') | |
| } | |
| logger.debug(service_account_info_json) | |
| credentials = service_account.Credentials.from_service_account_info(service_account_info_json) | |
| client = translate.TranslationServiceClient(credentials=credentials) | |
| location = "global" | |
| parent = f"projects/{Utility.environment['multilingual']['project_id']}/locations/{location}" | |
| response = client.get_supported_languages(parent=parent) | |
| response = client.get_supported_languages(parent=parent, display_language_code="en") | |
| result = {language.language_code: language.display_name for language in response.languages} | |
| return result | |
| @staticmethod | |
| def get_supported_languages(display_language_code: Text = "en") -> dict: | |
| """ | |
| Get supported languages with their display names. | |
| Args: | |
| display_language_code: Language code for the display names (default: "en") | |
| Returns: | |
| dict: Mapping of language codes to their display names | |
| Raises: | |
| AppException: If the API call fails or configuration is invalid | |
| """ | |
| multilingual_env = Utility.environment.get('multilingual', {}) | |
| service_account_creds = multilingual_env.get('service_account_creds', {}) | |
| service_account_info_json = { | |
| "type": service_account_creds.get('type', "service_account"), | |
| "project_id": multilingual_env.get('project_id'), | |
| "private_key_id": service_account_creds.get('private_key_id'), | |
| "private_key": service_account_creds.get('private_key'), | |
| "client_email": service_account_creds.get('client_email'), | |
| "client_id": service_account_creds.get('client_id'), | |
| "auth_uri": service_account_creds.get('auth_uri'), | |
| "token_uri": service_account_creds.get('token_uri'), | |
| "auth_provider_x509_cert_url": service_account_creds.get('auth_provider_x509_cert_url'), | |
| "client_x509_cert_url": service_account_creds.get('client_x509_cert_url') | |
| } | |
| logger.debug(service_account_info_json) | |
| credentials = service_account.Credentials.from_service_account_info(service_account_info_json) | |
| client = translate.TranslationServiceClient(credentials=credentials) | |
| location = "global" | |
| parent = f"projects/{Utility.environment['multilingual']['project_id']}/locations/{location}" | |
| response = client.get_supported_languages(parent=parent, display_language_code="en") | |
| result = {language.language_code: language.display_name for language in response.languages} | |
| return result |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/unit_test/multilingual/multilingual_processor_test.py (2)
588-592: Remove duplicate initialization in mock Language classThe language_code and display_name are initialized twice. Consider simplifying:
class Language: - language_code = "en" - display_name = "English" - def __init__(self): self.language_code = "en" self.display_name = "English"
608-608: Add test cases for multiple languagesThe current test only verifies a single language. Consider adding test cases for:
- Multiple languages
- Error scenarios
- Different display languages
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
kairon/shared/multilingual/utils/translator.py(1 hunks)tests/unit_test/multilingual/multilingual_processor_test.py(2 hunks)
🔇 Additional comments (3)
kairon/shared/multilingual/utils/translator.py (3)
72-85: LGTM! Consider adding validation for required credentials.
The refactoring improves error handling by using .get() with defaults. However, as suggested in a previous review, consider adding validation for required credentials.
93-95:
Add error handling and make display language configurable
While the dictionary format is an improvement, consider:
- Adding error handling for API calls
- Making the display language configurable
-response = client.get_supported_languages(parent=parent, display_language_code="en")
-result = {language.language_code: language.display_name for language in response.languages}
-return result
+try:
+ display_language = multilingual_env.get('display_language_code', 'en')
+ response = client.get_supported_languages(
+ parent=parent,
+ display_language_code=display_language
+ )
+ return {
+ language.language_code: language.display_name
+ for language in response.languages
+ }
+except Exception as e:
+ logger.exception(e)
+ raise AppException(f'Failed to fetch supported languages: {str(e)}')92-92:
Validate project_id before use
The project_id is required for the API call. Consider validating its presence before use to prevent runtime errors.
-parent = f"projects/{multilingual_env.get('project_id')}/locations/{location}"
+project_id = multilingual_env.get('project_id')
+if not project_id:
+ raise AppException("Missing required configuration: multilingual.project_id")
+parent = f"projects/{project_id}/locations/{location}"Likely invalid or redundant comment.
Summary by CodeRabbit
New Features
get_supported_languagesmethod to return a dictionary mapping language codes to their display names.Bug Fixes
Tests