feat: introduce new error system - #4479
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new error handling system that supports deferred translation of error messages, allowing errors to be translated based on the user's language preference at the point of display rather than at creation.
Changes:
- Introduces
TranslatableErrortype ini18n/util.goto store translation keys and parameters for deferred translation - Adds
ResponseTErrormethod incontrollers/util.goto automatically handle and translateTranslatableErrorinstances - Provides convenience function
i18n.T()for easy creation of translatable errors
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| i18n/util.go | Adds TranslatableError type with Error() and TranslateWithLanguage() methods, plus convenience function T() for creating translatable errors |
| controllers/util.go | Adds ResponseTError method to automatically translate TranslatableError types based on user's Accept-Language header |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // ResponseErrorFromError handles errors, automatically translating TranslatableError types | ||
| // Usage: c.ResponseErrorFromError(err) |
There was a problem hiding this comment.
The comment mentions "ResponseErrorFromError" but the actual function name is "ResponseTError". The documentation should be updated to match the function name for clarity.
| // Usage: c.ResponseErrorFromError(err) | |
| // Usage: c.ResponseTError(err) |
| } | ||
| return keyWithoutCategory | ||
| } | ||
|
|
There was a problem hiding this comment.
The TranslateWithLanguage method lacks documentation. As a public method of the TranslatableError type, it should have a comment describing its purpose, parameters, and return value, following the same pattern as the Error method.
| // TranslateWithLanguage returns the translated error message in the specified language. | |
| // It uses the TranslatableError's key and parameters to format the translated message. |
| func (te *TranslatableError) Error() string { | ||
| firstColonIndex := strings.Index(te.Key, ":") | ||
| if firstColonIndex == -1 { | ||
| // If no ":" is found, format with params if any | ||
| if len(te.Params) > 0 { | ||
| return fmt.Sprintf(te.Key, te.Params...) | ||
| } | ||
| return te.Key | ||
| } | ||
| // Extract the part after the first ":" | ||
| keyWithoutCategory := te.Key[firstColonIndex+1:] | ||
| if len(te.Params) > 0 { | ||
| return fmt.Sprintf(keyWithoutCategory, te.Params...) | ||
| } | ||
| return keyWithoutCategory | ||
| } |
There was a problem hiding this comment.
The Error() method uses fmt.Sprintf with te.Params without validation. If the Key format string and Params don't match (e.g., Key contains two %s but only one parameter is provided, or parameters have incompatible types), this will panic at runtime. Consider adding error handling or validation to prevent panics, similar to how the existing Translate function handles errors by returning error messages instead of panicking.
| func T(key string, params ...interface{}) *TranslatableError { | ||
| return NewTranslatableError(key, params...) | ||
| } |
There was a problem hiding this comment.
The function name "T" is very terse and could be ambiguous, especially since there's already a method "T" on ApiController that performs immediate translation. While the namespace (i18n.T vs c.T) differentiates them, consider a more descriptive name like "NewError" or "NewTranslatableError" for better clarity and to avoid potential confusion about which "T" function is being used.
| // TranslatableError is an error type that can be translated when needed | ||
| // It stores the translation key and parameters, allowing translation to happen | ||
| // at the point where the error is displayed, not when it's created | ||
| type TranslatableError struct { | ||
| Key string | ||
| Params []interface{} | ||
| } | ||
|
|
||
| // NewTranslatableError creates a new TranslatableError with the given key and parameters | ||
| // The key should be in the format "namespace:message" (e.g., "general:User not found") | ||
| // Parameters can be used for formatting the translated message | ||
| func NewTranslatableError(key string, params ...interface{}) *TranslatableError { | ||
| return &TranslatableError{ | ||
| Key: key, | ||
| Params: params, | ||
| } | ||
| } | ||
|
|
||
| // Error implements the error interface, returning the untranslated key | ||
| // This is useful for logging or when translation context is not available | ||
| func (te *TranslatableError) Error() string { | ||
| firstColonIndex := strings.Index(te.Key, ":") | ||
| if firstColonIndex == -1 { | ||
| // If no ":" is found, format with params if any | ||
| if len(te.Params) > 0 { | ||
| return fmt.Sprintf(te.Key, te.Params...) | ||
| } | ||
| return te.Key | ||
| } | ||
| // Extract the part after the first ":" | ||
| keyWithoutCategory := te.Key[firstColonIndex+1:] | ||
| if len(te.Params) > 0 { | ||
| return fmt.Sprintf(keyWithoutCategory, te.Params...) | ||
| } | ||
| return keyWithoutCategory | ||
| } | ||
|
|
||
| func (te *TranslatableError) TranslateWithLanguage(language string) string { | ||
| translatedFormat := Translate(language, te.Key) | ||
| if len(te.Params) > 0 { | ||
| return fmt.Sprintf(translatedFormat, te.Params...) | ||
| } | ||
| return translatedFormat | ||
| } | ||
|
|
||
| // T is a convenience function that creates a TranslatableError | ||
| // Usage: return i18n.T("general:User not found", userId) | ||
| func T(key string, params ...interface{}) *TranslatableError { | ||
| return NewTranslatableError(key, params...) | ||
| } |
There was a problem hiding this comment.
The i18n package has existing test coverage (see generate_test.go), but the new TranslatableError functionality lacks test coverage. Consider adding tests to verify: 1) Error() method correctly formats with and without parameters, 2) TranslateWithLanguage() correctly translates and formats messages, 3) T() convenience function works as expected, and 4) edge cases like mismatched format strings and parameters are handled properly.
| func (te *TranslatableError) TranslateWithLanguage(language string) string { | ||
| translatedFormat := Translate(language, te.Key) | ||
| if len(te.Params) > 0 { | ||
| return fmt.Sprintf(translatedFormat, te.Params...) | ||
| } | ||
| return translatedFormat | ||
| } |
There was a problem hiding this comment.
When the Translate function encounters errors (e.g., missing ":" in key, unsupported language), it returns error messages like "Translate error: the error text doesn't contain ":"...". The TranslateWithLanguage method then attempts to use this error message as a format string in fmt.Sprintf with te.Params. This could lead to mismatched format specifiers or unexpected panics. Consider checking if the translated result is an error message before applying fmt.Sprintf, or handle translation errors more explicitly.
288dea6 to
f46b92d
Compare
941f460 to
9038d8a
Compare
8adca86 to
7caec83
Compare
3b5ebe1 to
0e4d820
Compare
#4375
Changes
TranslatableErrortype (i18n/util.go): An error type that stores translation keys and parameters, allowing translation to happen when the error is displayedResponseTErrormethod (controllers/util.go): Automatically handlesTranslatableErrortypes and translates them based on the user's languagei18n.T(): Provides a simple way to createTranslatableErrorinstancesUsage
TODO
i18n.T()regexes to capture messages