fix: default en_US.UTF-8 if lang has problems #1687
Conversation
Mac's `defaults read -g AppleLocale` command can return locales that don't have a language in /usr/share/locale. This prevents those locales from using a language with UTF-8 support. This will use en_US.UTF-8 as a default to cover those cases if there are problems. As per usual, if LANG is already set, that value will be used instead.
WalkthroughThe changes in the 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: 0
🧹 Nitpick comments (2)
pkg/wavebase/wavebase.go (2)
204-213: Consider caching and filtering results.Although this function returns valid locales by calling
locale -a, it might be beneficial to:
- Cache the results to avoid multiple external calls if used repeatedly.
- Filter out empty lines or malformed entries to ensure only valid locales are returned.
219-226: Add a check for an existing LANG environment variable.The PR objectives mention preserving the user’s LANG setting if it is already defined. However, when
defaults read -g AppleLocalefails, the logic directly falls back to"en_US.UTF-8". You might want to first check ifLANGis set before forcing this fallback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/wavebase/wavebase.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
pkg/wavebase/wavebase.go (2)
20-21: No issues with the new import.The import for
utilfnis necessary forContainsStr. This looks fine as is.
230-238: Confirm consistent fallback logic across OSes.The code checks if
preferredLangis in the valid locale list and falls back toen_US.UTF-8otherwise. This is good for macOS, but Windows and other systems might need equivalent checks. Ensuring a consistent approach across all platforms can prevent locale mismatches.You can verify usage across OS detection points with the following script:
✅ Verification successful
Fallback logic is consistent across platforms ✅
The code implements a uniform approach for locale validation and fallback across all supported platforms:
- macOS: Uses AppleLocale with UTF-8 validation
- Windows: Uses Get-Culture with UTF-8 validation
- All platforms: Fall back to 'en_US.UTF-8' when validation fails
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for platform-specific code paths in wavebase.go rg -A 5 'runtime.GOOS'Length of output: 4855
The windows LANG code used the incorrect runtime.GOOS name for go, so it never actually ran. That said, international characters still seem to work fine on windows, so there is no harm in removing this entirely.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/wavebase/wavebase.go (3)
204-213: Consider caching the valid languages list.Since the available locales rarely change during runtime, consider caching the results using a similar pattern to
osLangOnceto avoid repeated system calls.+var validLangsOnce = &sync.Once{} +var validLangs []string + func listValidLangs(ctx context.Context) []string { + validLangsOnce.Do(func() { out, err := exec.CommandContext(ctx, "locale", "-a").CombinedOutput() if err != nil { log.Printf("error running 'locale -a': %s\n", err) - return []string{} + validLangs = []string{} + return } - return strings.Split(string(out), "\n") + // Filter out empty strings from the split result + allLangs := strings.Split(string(out), "\n") + validLangs = make([]string, 0, len(allLangs)) + for _, lang := range allLangs { + if lang != "" { + validLangs = append(validLangs, lang) + } + } + }) + return validLangs }
211-211: Enhance the Windows compatibility comment.The current comment could be more explicit about Windows compatibility.
-// this command doesn't work on windows +// Note: The 'locale -a' command is not available on Windows systems
230-238: Consider handling edge cases in language processing.The current implementation might benefit from additional validation:
- Check if the truncated output is empty
- Validate the UTF-8 suffix format
- preferredLang := strings.TrimSpace(truncOut) + ".UTF-8" + truncOut = strings.TrimSpace(truncOut) + if truncOut == "" { + log.Printf("empty locale detected, will use default %s\n", defaultLang) + return defaultLang + } + + // Ensure proper format for UTF-8 suffix + preferredLang := truncOut + if !strings.Contains(preferredLang, ".") { + preferredLang += ".UTF-8" + } - validLangs := listValidLangs(ctx) if !utilfn.ContainsStr(validLangs, preferredLang) { - log.Printf("unable to use desired lang %s, will use default 'en_US.UTF-8'\n", preferredLang) + log.Printf("desired language %s is not available, will use %s\n", preferredLang, defaultLang) return defaultLang }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/wavebase/wavebase.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (2)
pkg/wavebase/wavebase.go (2)
20-21: LGTM!The addition of the
utilfnpackage is appropriate for the new language validation functionality.
204-238: Overall implementation aligns with PR objectives.The changes successfully implement the fallback to en_US.UTF-8 when problems are detected with the specified locale. The implementation includes proper validation against available locales and handles macOS-specific locale detection as intended.
The suggested improvements above would make the implementation more robust, but the current implementation satisfies the core requirements.
| defaultLang := "en_US.UTF-8" | ||
| ctx, cancelFn := context.WithTimeout(context.Background(), 2*time.Second) | ||
| defer cancelFn() | ||
| if runtime.GOOS == "darwin" { | ||
| out, err := exec.CommandContext(ctx, "defaults", "read", "-g", "AppleLocale").CombinedOutput() | ||
| if err != nil { | ||
| log.Printf("error executing 'defaults read -g AppleLocale': %v\n", err) | ||
| return "" | ||
| log.Printf("error executing 'defaults read -g AppleLocale', will use default 'en_US.UTF-8': %v\n", err) | ||
| return defaultLang |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and validation.
A few suggestions to enhance the robustness of the language determination:
- The error message could be more informative by including the actual error output.
- The default language should also be validated against available locales.
func determineLang() string {
defaultLang := "en_US.UTF-8"
ctx, cancelFn := context.WithTimeout(context.Background(), 2*time.Second)
defer cancelFn()
+ validLangs := listValidLangs(ctx)
+
+ // Validate that default language is available
+ if !utilfn.ContainsStr(validLangs, defaultLang) {
+ log.Printf("warning: default language %s is not available, falling back to C.UTF-8\n", defaultLang)
+ defaultLang = "C.UTF-8"
+ }
+
if runtime.GOOS == "darwin" {
out, err := exec.CommandContext(ctx, "defaults", "read", "-g", "AppleLocale").CombinedOutput()
if err != nil {
- log.Printf("error executing 'defaults read -g AppleLocale', will use default 'en_US.UTF-8': %v\n", err)
+ log.Printf("error executing 'defaults read -g AppleLocale', will use %s: %v (output: %s)\n",
+ defaultLang, err, string(out))
return defaultLang
}Committable suggestion skipped: line range outside the PR's diff.
Mac's
defaults read -g AppleLocalecommand can return locales that don't have a language in /usr/share/locale. This prevents those locales from using a language with UTF-8 support. This will use en_US.UTF-8 as a default to cover those cases if there are problems. As per usual, if LANG is already set, that value will be used instead.