-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat: Add Multi-Language Support for Application Labels #7714
Conversation
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Sort: t.Sort, | ||
Key: t.Key, | ||
Translations: string(translations), | ||
Sort: t.Sort, | ||
}) | ||
} | ||
oldApps, err := appRepo.GetBy(appRepo.WithResource(constant.AppResourceRemote)) |
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.
The provided Go code has several improvements to ensure consistency, readability, and functionality. Here are the specific changes:
-
Import Adjustments:
- Import
github.com/gin-gonic/gin
instead of importing all packages at once.
- Import
-
Method Parameters:
- Changed method parameters to use pointers (
*request.AppSearch
) to avoid unnecessary copying and potential memory allocation problems.
- Changed method parameters to use pointers (
-
Loop Variables:
- Removed unused variables from loops that did not contribute to meaningful computation.
-
Error Handling for Unmarshaling:
- Added error handling after unmarshalling JSON data using
_ = ...
.
- Added error handling after unmarshalling JSON data using
-
Language Support:
- Extracted language support into a utility function such as
common.GetLang
, which reduces repetition and provides better abstraction.
- Extracted language support into a utility function such as
-
String Case Conversion:
- Used
strings.ToLower()
consistently for lowercase conversions.
- Used
-
Consistent Return Type:
- Ensured consistent return types across similar methods where applicable.
Here is a concise summary of these changes:
-
Refactored Imports: Simplified imports by splitting them into separate lines for clarity.
-
Updated Method Signatures: Enhanced method signatures with pointer receivers for better performance when modifying object instances within methods.
-
Removed Unused Variables: Eliminated unnecessary intermediate variable assignments, especially in looping constructs like
for-loop
s. -
Enhanced Error Handling: Added more detailed error checks in certain parts, specifically related to JSON parsing operations.
These modifications can greatly enhance the reliability, maintainability, and performance of the application, making it easier to read, debug, and optimize.
model.Tag | ||
ID uint `json:"id"` | ||
Key string `json:"key"` | ||
Name string `json:"name"` | ||
} | ||
|
||
type AppInstalledCheck struct { |
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.
The provided code differs slightly between the two struct definitions. In the first version, you have used model.Tag
directly in the Tags
field of the AppDTO
, while in the second version, you have replaced it with an embedded TagDTO
. This change requires corresponding adjustments to handle serialization and deserialization.
Here is the corrected and optimized version:
// Corrected tags field in AppDTO
type AppDTO struct {
Name string `json:"name"`
Key string `json:"key"`
ID uint `json:"id"`
ShortDescZh string `json:"shortDescZh"`
ShortDescEn string `json:"shortDescEn"`
Icon string `json:"icon"`
Type string `json:"type"`
Status string `json:"status"`
Resource string `json:"resource"`
Installed bool `json:"installed"`
Versions []string `json:"versions"`
Limit int `json:"limit"`
Tags []TagDTO `json:"tags"` // Embedded TagDTO now
}
// Optimized TagDTO structure (not required if using Model.Tag)
type TagDTO struct {
ID uint `json:"id"` // Optional if using model.Tag
Key string `json:"key"`
Name string `json:"name"`
}
Key Changes:
- Embedded vs Direct Usage: The
Tags
field inAppDTO
was changed from[]model.Tag
to[]TagDTO
. - Optional Fields: If you intend to use
model.Tag
, you can remove the optional fields (ID
,Key
, etc.) inTagDTO
. However, if not needed, keeping them provides more flexibility for customizing the tag data. - Serialization/Deserialization: Ensure that both
TagDTO
andmodel.Tag
implement the necessary interfaces (like marshaling and unmarshaling) so as not to cause errors during JSON operations.
This ensures proper handling and compatibility between the structs, especially when dealing with JSON encoding/decoding.
backend/app/dto/app.go
Outdated
Sort int `json:"sort"` | ||
Translations Translation `json:"translations"` | ||
} | ||
|
||
type AppForm struct { | ||
FormFields []AppFormFields `json:"formFields"` | ||
SupportVersion float64 `json:"supportVersion"` |
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.
The provided code has a few areas that can be improved:
Type Definitions
- ExtraProperties Struct: The
Tags
field is now a slice ofTag
, which seems correct. - AppProperty Struct: This structure does not contain any changes, so it should remain unchanged.
- Translation Struct: A new struct
Translation
has been introduced for handling translations in multiple languages.
type Translation struct {
En string `json:"en"`
Ja string `json:"ja"`
Ms string `json:"ms"`
PtBr string `json:"pt-br"`
Ru string `json:"ru"`
Tw string `json:"tw"`
Zh string `json:"zh"`
}
- AppGroup Struct: Introduced a new
Groups
field to store additional groups within an application and included theTranslations
field to extend the translation support within these groups.
type AppGroup struct {
Key string `json:"key"` // Unique identifier for the group
Sort int `json:"sort"` // Position order within the app's categories/groups list
Translations Translation `json:"translations"` // Additional text properties per language
}
- AppForm Struct: No significant changes identified.
Potential Improvements
- Ensure all dependencies used throughout this structure are properly imported at the beginning.
- Consider using pointers for fields like
Key
,Sort
,SupportVersion
if they might ever need to be updated or modified after initialization. - For performance reasons, you might consider using more efficient container types if applicable (e.g., slices over arrays).
- Add documentation comments to each struct member to maintain clarity on their purposes and usage within different parts of the system.
These improvements will enhance readability, flexibility, and efficiency while maintaining the integrity and correctness of your data structure. If there are specific areas of concern or further requirements, please let me know!
e2494bc
to
daaa08a
Compare
Sort: t.Sort, | ||
Key: t.Key, | ||
Translations: string(translations), | ||
Sort: t.Sort, | ||
}) | ||
} | ||
oldApps, err := appRepo.GetBy(appRepo.WithResource(constant.AppResourceRemote)) |
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.
There are no irregularities or potential issues in the provided Go code snippets, and no specific optimization suggestions.
model.Tag | ||
ID uint `json:"id"` | ||
Key string `json:"key"` | ||
Name string `json:"name"` | ||
} | ||
|
||
type AppInstalledCheck struct { |
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.
The provided code differences seem minor but can be considered for some improvements. Here's an optimized version with additional comments:
package model
import (
)
type AppDTO struct {
}
type AppItem struct {
Name string `json:"name"`
Key string `json:"key"`
ID uint `json:"id"`
ShortDescZh string `json:"shortDescZh"`
ShortDescEn string `json:"shortDescEn"`
Icon string `json:"icon"`
Type string `json:"type"`
Status string `json:"status"`
Resource string `json:"resource"`
Installed bool `json:"installed"`
Versions []string `json:"versions"`
Limit int `json:"limit"`
Tags []TagDTO `json:"tags"` // Optimized to use interface
}
// TagDTO includes only ID, Key, and Name fields which represent core tag properties.
type TagDTO struct {
ID uint `json:"id" form:"-"`
Key string `json:"key" form:"-" xml:"-" swaggerignore`
Name string `json:"name" form:"-" xml:"-"`
}
type AppInstalledCheck struct {
}
Suggestions:
- Interface Usage: The
Tags
field is now defined as[]TagDTO
, assuming thatTagDTO
has a method signature appropriate for its intended usage (e.g., comparison). - Simplification of
AppItem
: While not strictly necessary without additional context, simplifying theAppItem
structure by using a common base could improve readability.
These changes make the code easier to understand and maintain while maintaining functionality.
PtBr string `json:"pt-br"` | ||
Ru string `json:"ru"` | ||
ZhHant string `json:"zh-hant"` | ||
Zh string `json:"zh"` | ||
} | ||
|
||
type AppForm struct { |
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.
Your proposed changes seem to be correct and align with typical Go struct definitions and JSON serialization practices. Here's a quick review of what they entail:
-
Locale Struct: This addition allows tags to include localized text fields (e.g., "en", "ja", etc.), which can be useful for displaying names or descriptions in different languages.
-
Optimization Suggestions:
- Since you've added this feature but haven't used it, consider whether locale information is frequently accessed alongside other tag attributes. If not, potentially remove or refactor this functionality.
-
Potential Issues:
- Ensure that if you plan to implement locale localization, there are appropriate methods or hooks to retrieve translated strings.
- Verify compatibility across multiple versions of your application where locales might need to change dynamically.
Overall, these changes maintain data integrity and flexibility by allowing tagging systems to support multilingual content. Let me know if you need further assistance!
Quality Gate passedIssues Measures |
No description provided.