More server i18n #30
More server i18n #30
Conversation
WalkthroughThis pull request introduces several changes across multiple files, primarily focusing on enhancing localization support and improving type annotations. Key updates include the addition of the Changes
Possibly related PRs
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: 7
🧹 Outside diff range and nitpick comments (18)
app/translations/__init__.py (1)
21-28: Debug logs will be suppressed due to logging level changeThe
logging.debugstatements within this block will not be output because the logging level is now set toINFO. If these logs are important for diagnosing locale issues, consider changing them tologging.infoor adjusting the logging configuration.Apply this diff to adjust the logging statements:
-logging.debug( +logging.info( "%s set locale=%s from user pref", req_header, current_user.locale )And
-logging.debug("%s set locale=%s from req", req_header, best_match) +logging.info("%s set locale=%s from req", req_header, best_match)app/tasks/import_from_labelplus.py (2)
59-59: Remove redundant check fortargetandcreatorvariablesThe variables
targetandcreatorhave already been validated before thetryblock. The conditionif target and creator:at line 59 is redundant and can be removed to simplify the code.Apply this diff to remove the redundant condition:
- if target and creator: project.update( import_from_labelplus_percent=0, import_from_labelplus_status=ImportFromLabelplusStatus.RUNNING, )
59-83: Simplify indentation after removing redundant conditionAfter removing the redundant
ifcondition, adjust the indentation of the subsequent code block for proper structure.Apply this diff to adjust indentation:
- if target and creator: project.update( import_from_labelplus_percent=0, import_from_labelplus_status=ImportFromLabelplusStatus.RUNNING, ) labelplus_data = load_from_labelplus(project.import_from_labelplus_txt) file_count = len(labelplus_data) for file_index, labelplus_file in enumerate(labelplus_data):app/config.py (1)
Line range hint
81-81: Simplify boolean environment variable parsingThe current pattern for parsing boolean environment variables can be simplified and made more robust against case variations.
Consider defining a helper function to parse boolean values:
def get_env_bool(key, default=False): return env.get(key, str(default)).lower() in ("true", "1", "yes")Then, update the boolean assignments:
-OSS_VIA_CDN = True if env.get("OSS_VIA_CDN", "") == "True" else False +OSS_VIA_CDN = get_env_bool("OSS_VIA_CDN", False)Apply similar changes to other boolean configurations for consistency and clarity.
app/translations/en/LC_MESSAGES/messages.po (12)
467-467: Ensure Accurate Translation for '无需审核'The translation of "无需审核" has been changed to "Auto approve." While this conveys a similar meaning, "No approval required." or "No review required." might be more precise translations.
Apply this change:
- msgstr "Auto approve." + msgstr "No approval required."
475-475: Improve Translation for ClarityThe translation "Need Admin Review" for "管理员审核" may benefit from slight adjustment for grammatical correctness. Consider changing it to "Admin Review Required" or "Administrator Approval Needed."
Apply this change:
- msgstr "Need Admin Review" + msgstr "Admin Review Required"
519-519: Correct Grammatical Error in TranslationThe translation "You can only remove users with less powerful role than your own." should be "You can only remove users with roles less powerful than your own."
Apply this change:
- msgstr "You can only remove users with less powerful role than your own." + msgstr "You can only remove users with roles less powerful than your own."
527-527: Correct Grammatical Error in TranslationThe phrase "You can only modify users with less powerful role than your own." should be "You can only modify users with roles less powerful than your own."
Apply this change:
- msgstr "You can only modify users with less powerful role than your own." + msgstr "You can only modify users with roles less powerful than your own."
583-583: Improve Translation for AccuracyThe translation "You can only assign roles that are lower than your own." may be clearer as "You can only assign roles with levels lower than your own."
Apply this change:
- msgstr "You can only assign roles that are lower than your own." + msgstr "You can only assign roles with levels lower than your own."
635-635: Clarify Translation WordingThe translation "Only Chinese/Japanese/Korean/English alphabets/numbers/_ can be used." could be simplified for clarity. Consider "Only letters (Chinese, Japanese, Korean, English), numbers, or underscores (_) are allowed."
Apply this change:
- msgstr "Only Chinese/Japanese/Korean/English alphabets/numbers/_ can be used." + msgstr "Only letters (Chinese, Japanese, Korean, English), numbers, or underscores (_) are allowed."
997-997: Modify Translation for ConsistencyThe phrase "Already approved by others." could be rephrased to "Approved by someone else." for better clarity.
Apply this change:
- msgstr "Already approved by others." + msgstr "Approved by someone else."
1333-1333: Correct Typographical Error in TranslationThe translation "Illegal auth token. It should be like Bearer x.x.x." contains a minor issue. Consider changing "like Bearer x.x.x." to "in the format 'Bearer x.x.x'."
Apply this change:
- msgstr "Illegal auth token. It should be like Bearer x.x.x." + msgstr "Invalid auth token. It should be in the format 'Bearer x.x.x'."
1337-1337: Improve Translation for ClarityThe message "Password updated. Please log in again." is clear but could emphasize urgency. Consider "Password has been changed. Please log in again."
Apply this change:
- msgstr "Password updated. Please log in again." + msgstr "Password has been changed. Please log in again."
1349-1349: Enhance Translation Tone"Invited. Please wait for the user to confirm." could be improved for a more formal tone. Consider "Invitation sent. Please wait for the user's confirmation."
Apply this change:
- msgstr "Invited. Please wait for the user to confirm." + msgstr "Invitation sent. Please wait for the user's confirmation."
1365-1365: Standardize TranslationThe message "Successfully joined." could be standardized to match other success messages like "Joined successfully."
Apply this change:
- msgstr "Successfully joined." + msgstr "Joined successfully."
1369-1369: Correct Grammatical Structure"Application Sent. Please wait for administrator to review." should include an article before "administrator." Consider "Please wait for the administrator to review."
Apply this change:
- msgstr "Application Sent. Please wait for administrator to review." + msgstr "Application sent. Please wait for the administrator to review."app/models/file.py (1)
1226-1226: Instantiate exceptions when raising them for consistency.When raising
SourceNotExistError, it's a good practice to instantiate the exception using parentheses to ensure any initialization code is executed.Apply this diff:
if source is None: - raise SourceNotExistError + raise SourceNotExistError()app/tasks/output_team_projects.py (1)
Line range hint
11-13: Review logging strategy for i18n environmentWhile user-facing messages should be translated, log messages should remain in a consistent language (typically English) for easier debugging and log analysis. Consider documenting this requirement in the logging setup.
Consider adding a comment to clarify the logging language policy:
logger = get_task_logger(__name__) +# Note: Log messages should be in English regardless of user's locale
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
.env.sample(1 hunks)app/apis/me.py(1 hunks)app/config.py(1 hunks)app/core/rbac.py(1 hunks)app/factory.py(3 hunks)app/models/__init__.py(1 hunks)app/models/file.py(1 hunks)app/models/project.py(1 hunks)app/models/team.py(2 hunks)app/tasks/import_from_labelplus.py(1 hunks)app/tasks/output_team_projects.py(1 hunks)app/translations/__init__.py(2 hunks)app/translations/en/LC_MESSAGES/messages.po(12 hunks)app/translations/zh/LC_MESSAGES/messages.po(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/core/rbac.py
- app/translations/zh/LC_MESSAGES/messages.po
🔇 Additional comments (13)
app/models/__init__.py (1)
9-9: Verify the impact of changing logging level to INFO
Changing the logging level from DEBUG to INFO will suppress debug messages. Ensure that this change does not hinder debugging or monitoring critical database operations.
Run the following script to identify all logging.debug statements in the codebase:
✅ Verification successful
Changing logging level to INFO is acceptable
The debug messages found in the codebase are non-critical and only used for development/troubleshooting purposes:
- Locale selection logging in translations
- Logger configuration details
- File operations tracking (OSS service)
- Project file creation tracing
These debug statements are used for tracing program flow and don't contain critical operational data that would impact system monitoring or debugging of production issues. The INFO level is appropriate for production environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all 'logging.debug' statements in the codebase.
rg 'logging\.debug'
Length of output: 415
Script:
#!/bin/bash
# Get more context around the logging.debug statements to understand their importance
rg 'logging\.debug' -B 2 -A 2
Length of output: 1838
app/translations/__init__.py (4)
41-47: Addition of gettext function enhances translation handling
The gettext function wraps flask_babel.gettext and adds logging for translation events. Ensure that the logging level allows these logs to be captured if needed for debugging.
49-52: Addition of lazy_gettext function provides lazy translations
The lazy_gettext function correctly implements lazy evaluation of translations using flask_babel.LazyString. The commented-out debug log suggests it might be useful; consider enabling it if necessary.
8-8: Verify the impact of changing logging level to INFO
Changing the logging level from DEBUG to INFO will suppress debug messages in the translations module. Ensure that essential debug logs for localization are not being unintentionally suppressed.
Run the following script to identify logging.debug statements in app/translations/__init__.py:
✅ Verification successful
Changing logging level to INFO is acceptable
The debug logs in the translations module are only used for tracing locale selection logic and don't contain essential operational information. The first debug statement is in a code block that's explicitly noted as unused (commented as "won't get called"), and the second one simply logs the selected locale from request headers. Suppressing these debug messages won't impact the system's functionality or observability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find 'logging.debug' statements in app/translations/__init__.py
rg 'logging\.debug' app/translations/__init__.py
Length of output: 144
Script:
#!/bin/bash
# Get more context around the debug statements to understand their importance
rg 'logging\.debug' app/translations/__init__.py -B 2 -A 2
Length of output: 464
11-11:
Ensure all references to get_locale are updated
The function get_locale has been renamed to get_request_locale. Please ensure that all references to get_locale in the codebase are updated to prevent any broken functionality.
Run the following script to find any remaining references to get_locale:
.env.sample (1)
16-17: Addition of BABEL_DEFAULT_LOCALE variable improves configurability
Introducing BABEL_DEFAULT_LOCALE allows for better control over the application's default locale through environment configuration. Ensure that this variable is documented and that default values are handled appropriately in the application.
app/config.py (1)
39-39: Set default locale from environment variable
The addition of BABEL_DEFAULT_LOCALE = env.get("BABEL_DEFAULT_LOCALE", "zh") enhances configurability by allowing the default locale to be set via the environment. Ensure that the environment variable BABEL_DEFAULT_LOCALE is correctly configured in your deployment environments.
app/factory.py (3)
2-2: Update import statement for Celery
Changing from celery import Celery to import celery is acceptable. Ensure that all references to Celery are updated accordingly to maintain consistency.
61-68: Ensure Celery tasks run within Flask application context
The introduction of the FlaskTask class, which overrides the __call__ method, ensures that tasks execute within the Flask application context. This change enhances the integration of Celery with Flask.
46-50: Enhance locale initialization with default_locale
Specifying default_locale in babel.init_app aligns with the new configuration in app.config.py and improves internationalization support.
app/models/team.py (1)
6-6: Verify Custom Translation Module Functionality
Importing lazy_gettext and gettext from app.translations suggests a shift from using flask_babel. Ensure that app.translations provides implementations consistent with flask_babel to prevent any unexpected behavior in translation functions.
app/tasks/output_team_projects.py (2)
40-41: Consider i18n configuration implications after app context removal
The removal of the Flask app context might affect how internationalization settings are loaded and accessed. Consider explicitly handling locale configuration to ensure consistent behavior.
Let's verify if there are any i18n-related configurations that might be affected:
43-64: 🛠️ Refactor suggestion
Internationalize user-facing messages
The function contains hardcoded Chinese strings that should be wrapped in translation functions for proper i18n support. This is especially important for user-facing messages in a system that's being internationalized.
Consider wrapping the return message in a translation function:
- return f"成功：已创建 Team <{str(team.id)}> 所有项目的导出任务"
+ return gettext("Success: Export tasks created for all projects in Team <%(team_id)s>") % {'team_id': str(team.id)}Additionally, consider adding translated error messages for potential failure cases in the project processing loop.
Likely invalid or redundant comment.
| project.update( | ||
| import_from_labelplus_percent=0, | ||
| import_from_labelplus_status=ImportFromLabelplusStatus.RUNNING, | ||
| import_from_labelplus_percent=int((file_index / file_count) * 100) |
There was a problem hiding this comment.
Correct progress percentage calculation
The calculation int((file_index / file_count) * 100) does not reach 100% on the last iteration because file_index starts from 0. Adjust the calculation to ensure accurate progress reporting.
Apply this diff to fix the progress percentage calculation:
import_from_labelplus_percent=int((file_index / file_count) * 100)
+ import_from_labelplus_percent=int(((file_index + 1) / file_count) * 100)This change ensures that the progress reaches 100% upon completion.
📝 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.
| import_from_labelplus_percent=int((file_index / file_count) * 100) | |
| import_from_labelplus_percent=int(((file_index + 1) / file_count) * 100) |
| labelplus_data = load_from_labelplus(project.import_from_labelplus_txt) | ||
| file_count = len(labelplus_data) |
There was a problem hiding this comment.
Handle empty data to prevent division by zero
If labelplus_data is empty, file_count becomes zero, leading to a potential ZeroDivisionError during progress calculation. Introduce a check to handle the scenario where there are no files to import.
Apply this diff to handle the zero files case:
labelplus_data = load_from_labelplus(project.import_from_labelplus_txt)
file_count = len(labelplus_data)
+ if file_count == 0:
+ project.update(
+ import_from_labelplus_status=ImportFromLabelplusStatus.ERROR,
+ import_from_labelplus_error_type=ImportFromLabelplusErrorType.NO_FILES,
+ )
+ return f"失败：没有文件可导入，Project {project_id}"📝 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.
| labelplus_data = load_from_labelplus(project.import_from_labelplus_txt) | |
| file_count = len(labelplus_data) | |
| labelplus_data = load_from_labelplus(project.import_from_labelplus_txt) | |
| file_count = len(labelplus_data) | |
| if file_count == 0: | |
| project.update( | |
| import_from_labelplus_status=ImportFromLabelplusStatus.ERROR, | |
| import_from_labelplus_error_type=ImportFromLabelplusErrorType.NO_FILES, | |
| ) | |
| return f"失败：没有文件可导入，Project {project_id}" |
| relations: list[TeamUserRelation] = TeamUserRelation.objects( | ||
| group__in=teams, user=self.current_user | ||
| ) |
There was a problem hiding this comment.
Ensure Type Hint Compatibility with Python Versions Below 3.9
The use of list[TeamUserRelation] as a type hint requires Python 3.9 or above. If your project supports earlier Python versions, consider using List[TeamUserRelation] from the typing module for compatibility.
Apply this diff to fix the type hinting:
- relations: list[TeamUserRelation] = TeamUserRelation.objects(
+ relations: List[TeamUserRelation] = TeamUserRelation.objects(Additionally, import List from the typing module at the beginning of the file:
from typing import List| @@ -470,7 +470,7 @@ def to_api(self, user=None): | |||
| # 如果给了 role 则获取用户相关信息（角色等） | |||
| role = None | |||
| if user: | |||
| role = user.get_role(self) | |||
| role: TeamRole | None = user.get_role(self) | |||
There was a problem hiding this comment.
Avoid Using Union Operator in Type Annotations for Python Versions Below 3.10
The type hint TeamRole | None utilizes the union operator |, which was introduced in Python 3.10. If the project needs to maintain compatibility with earlier Python versions, use Optional[TeamRole] from the typing module instead.
Apply this diff to fix the type hinting:
- role: TeamRole | None = user.get_role(self)
+ role: Optional[TeamRole] = user.get_role(self)Also, import Optional from the typing module:
from typing import Optional| @@ -508,33 +508,31 @@ msgstr "Invite User" | |||
|
|
|||
| #: app/core/rbac.py:107 | |||
| msgid "邀请时仅可设置比自己角色等级低的用户" | |||
| msgstr "You can only invite users with a lower role level than your own." | |||
| msgstr "You can only invite users as less powerful role than your own." | |||
There was a problem hiding this comment.
Correct Grammatical Error in Translation
The phrase "You can only invite users as less powerful role than your own." has grammatical issues. It should be revised to "You can only invite users to roles less powerful than your own."
Apply this change:
- msgstr "You can only invite users as less powerful role than your own."
+ msgstr "You can only invite users to roles less powerful than your own."📝 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.
| msgstr "You can only invite users as less powerful role than your own." | |
| msgstr "You can only invite users to roles less powerful than your own." |
| def by_id(cls, id: ObjectId): | ||
| project_set = cls.objects(id=id).first() | ||
| if project_set is None: | ||
| raise ProjectSetNotExistError() | ||
| return set | ||
| return project_set |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using id as a parameter name to prevent shadowing the built-in function.
Using id as a parameter name shadows the built-in function id(). This can lead to confusion and potential bugs. Consider renaming the parameter to id_ for clarity.
Apply this diff to rename the parameter and its usage:
@classmethod
-def by_id(cls, id: ObjectId):
- project_set = cls.objects(id=id).first()
+def by_id(cls, id_: ObjectId):
+ project_set = cls.objects(id=id_).first()
if project_set is None:
raise ProjectSetNotExistError()
return project_set📝 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.
| def by_id(cls, id: ObjectId): | |
| project_set = cls.objects(id=id).first() | |
| if project_set is None: | |
| raise ProjectSetNotExistError() | |
| return set | |
| return project_set | |
| def by_id(cls, id_: ObjectId): | |
| project_set = cls.objects(id=id_).first() | |
| if project_set is None: | |
| raise ProjectSetNotExistError() | |
| return project_set |
| def by_id(cls, id_: str | ObjectId) -> "Source": | ||
| source = cls.objects(id=id_).first() |
There was a problem hiding this comment.
Ensure compatibility with Python versions when using union type annotations with |.
The use of str | ObjectId for type annotations requires Python 3.10 or newer. If the codebase needs to support earlier Python versions, consider using Union[str, ObjectId] from the typing module.
Apply this diff to ensure compatibility:
+from typing import Union
...
@classmethod
- def by_id(cls, id_: str | ObjectId) -> "Source":
+ def by_id(cls, id_: Union[str, ObjectId]) -> "Source":
source = cls.objects(id=id_).first()
if source is None:
raise SourceNotExistErrorCommittable suggestion skipped: line range outside the PR's diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #30 +/- ##
==========================================
- Coverage 79.75% 78.83% -0.93%
==========================================
Files 109 110 +1
Lines 6689 6752 +63
==========================================
- Hits 5335 5323 -12
- Misses 1354 1429 +75 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
Release Notes
New Features
BABEL_DEFAULT_LOCALEset toen.gettextandlazy_gettextfunctions.Improvements
Bug Fixes
Documentation