fix: locale-dependent number/currency formatting.#20
fix: locale-dependent number/currency formatting.#20bmerkle wants to merge 1 commit intoAgentEvalHQ:mainfrom
Conversation
- The system locale uses e.g. , as decimal separator instead of . - fixed all affected classes/locations which e.g. produced 0.95 instead of 0,95 on comma-locale systems.
| { | ||
| var statusIcon = !call.HasError ? "✓" : "✗"; | ||
| var duration = call.Duration?.TotalMilliseconds.ToString("F0") + "ms" ?? "?ms"; | ||
| var duration = call.Duration?.TotalMilliseconds.ToString("F0", CultureInfo.InvariantCulture) + "ms" ?? "?ms"; |
There was a problem hiding this comment.
call.Duration?.TotalMilliseconds.ToString("F0", CultureInfo.InvariantCulture) is operating on a nullable (double?) and will not bind to the formatted ToString overload. Additionally, the + "ms" ?? "?ms" expression will never hit the ?? branch because string concatenation yields a non-null string even when the left side is null. Consider formatting via an explicit null check (e.g., use call.Duration is TimeSpan d / call.Duration.HasValue) so null durations render as ?ms and non-null values are formatted with InvariantCulture.
| var duration = call.Duration?.TotalMilliseconds.ToString("F0", CultureInfo.InvariantCulture) + "ms" ?? "?ms"; | |
| var duration = call.Duration is TimeSpan d | |
| ? d.TotalMilliseconds.ToString("F0", CultureInfo.InvariantCulture) + "ms" | |
| : "?ms"; |
| sb.AppendLine($"**Status:** {statusEmoji} {statusText}"); | ||
| sb.AppendLine($"**Score:** {report.OverallScore:F1}/100"); | ||
| sb.AppendLine($"**Tests:** {report.PassedTests}/{report.TotalTests} passed ({report.PassRate:F0}%)"); | ||
| sb.AppendLine(string.Create(CultureInfo.InvariantCulture, $"**Score:** {report.OverallScore:F1}/100")); | ||
| sb.AppendLine(string.Create(CultureInfo.InvariantCulture, $"**Tests:** {report.PassedTests}/{report.TotalTests} passed ({report.PassRate:F0}%)")); |
There was a problem hiding this comment.
PR description says comma-locale systems should display values like 0,95 (comma decimal separator), but the changes consistently force CultureInfo.InvariantCulture, which will output 0.95. Please clarify the intended behavior: either (a) update the PR description to reflect a goal of culture-invariant (dot) formatting for stable exports/logs, or (b) switch these user-facing strings to CultureInfo.CurrentCulture / CurrentUICulture where localized formatting is desired.
| sb.AppendLine(string.Create(CultureInfo.InvariantCulture, $"**Score:** {report.OverallScore:F1}/100")); | ||
| sb.AppendLine(string.Create(CultureInfo.InvariantCulture, $"**Tests:** {report.PassedTests}/{report.TotalTests} passed ({report.PassRate:F0}%)")); |
There was a problem hiding this comment.
These changes aim to make formatting culture-stable, but the existing exporter tests don’t appear to validate behavior under a non-dot decimal culture (e.g., de-DE). Consider adding a unit test that temporarily sets CultureInfo.CurrentCulture / CurrentUICulture to a comma-decimal locale and asserts the exported markdown still matches the intended format, to prevent regressions.
|
@copilot review again the PR code - is this still relevant? (I just merged a lot of code... so re-review everything) |
|
@copilot apply changes based on the comments in this thread |
Description
locale specific settings where not handeled
Motivation and Context
need for running also in other countries
Type of Change
How Has This Been Tested?
Test Configuration:
Test Details:
test run
Checklist
Screenshots (if applicable)
Additional Notes
Breaking Changes