-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: internationalize hardcoded error messages #7398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,13 @@ vi.mock("../../prompts/responses", () => ({ | |
|
|
||
| vi.mock("../../../i18n", () => ({ | ||
| t: vi.fn((key: string, params?: any) => { | ||
| // Handle the new tools error messages | ||
| if (key === "tools:errors.missingRequiredParameter.withPath" && params) { | ||
| return `Roo tried to use ${params.toolName} for '${params.relPath}' without value for required parameter '${params.paramName}'. Retrying...` | ||
| } | ||
| if (key === "tools:errors.missingRequiredParameter.withoutPath" && params) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good job updating the test mocks to handle the new i18n keys. This ensures the tests continue to work correctly with the internationalized messages. |
||
| return `Roo tried to use ${params.toolName} without value for required parameter '${params.paramName}'. Retrying...` | ||
| } | ||
| if (key === "mcp:errors.invalidJsonArgument" && params?.toolName) { | ||
| return `Roo tried to use ${params.toolName} with an invalid JSON argument. Retrying...` | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,5 +14,18 @@ | |||||
| "errors": { | ||||||
| "policy_restriction": "Failed to create new task due to policy restrictions." | ||||||
| } | ||||||
| }, | ||||||
| "errors": { | ||||||
| "missingRequiredParameter": { | ||||||
| "withPath": "Roo tried to use {{toolName}} for '{{relPath}}' without value for required parameter '{{paramName}}'. Retrying...", | ||||||
| "withoutPath": "Roo tried to use {{toolName}} without value for required parameter '{{paramName}}'. Retrying..." | ||||||
| }, | ||||||
| "lineCountMissing": "Roo tried to use write_to_file{{relPath}} but the required parameter 'line_count' was missing or truncated after {{actualLineCount}} lines of content were written. Retrying...", | ||||||
|
||||||
| "lineCountMissing": "Roo tried to use write_to_file{{relPath}} but the required parameter 'line_count' was missing or truncated after {{actualLineCount}} lines of content were written. Retrying...", | |
| "lineCountMissing": "Roo tried to use write_to_file {{relPath}} but the required parameter 'line_count' was missing or truncated after {{actualLineCount}} lines of content were written. Retrying...", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice that some error messages include HTML-like tags (<error_details>) while others don't. For consistency across the codebase, should we standardize whether error details are wrapped in these tags or not?
For example, fileNotFound includes the tags while fileNotFoundSimple doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestion: Several error messages follow similar patterns (e.g., "File does not exist at path: {{path}}"). In the future, these could potentially be further abstracted into a single parameterized message to reduce duplication. But this current approach is perfectly fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on adding these error message keys! The structure is clear and follows good i18n practices with parameter interpolation.
One observation: The PR description mentions that translations for the other 17 supported languages will be handled separately. Would it be helpful to create placeholder entries in the other locale files now, or track this as a follow-up task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactoring! The conditional logic for handling the path parameter makes the error messages more contextual. This is a good improvement over the original hardcoded message.