Add UseScaleWord format to auto-select scale based on locale#1697
Add UseScaleWord format to auto-select scale based on locale#1697reabr wants to merge 6 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d51f38952c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| var isLongScale = LongScaleCultures.Contains(culture.Name) | ||
| || LongScaleCultures.Contains(culture.TwoLetterISOLanguageName); |
There was a problem hiding this comment.
UseScaleWord treats a culture as long-scale when either culture.Name or culture.TwoLetterISOLanguageName is in LongScaleCultures. Because the set includes "pt", this forces pt-BR into long-scale even though Humanizer registers a separate Brazilian Portuguese converter (NumberToWordsConverterRegistry) and that converter uses short-scale billions (BrazilianPortugueseNumberToWordsConverter with bilhão at 10^9). In practice, 1E9.ToMetric(WithSpace | UseScaleWord) will return the wrong scale word for pt-BR.
Useful? React with 👍 / 👎.
| static readonly HashSet<string> LongScaleCultures = | ||
| [ | ||
| "de", "de-DE", // German | ||
| "fr", "fr-FR", // French | ||
| "it", "it-IT", // Italian |
There was a problem hiding this comment.
The hard-coded LongScaleCultures list is incomplete relative to locales already supported by the repo, so UseScaleWord silently falls back to short scale for some long-scale cultures. For example, Czech (cs) is registered and its converter uses miliarda for 10^9 (CzechNumberToWordsConverter), but cs is absent here, so cs-CZ will produce short-scale words. This makes the new “auto-select based on locale” behavior inaccurate for supported cultures.
Useful? React with 👍 / 👎.
reabr
force-pushed
the
feature/tometric-locale-scale
branch
from
0ed2d84 to
6483a84
Compare
|
Rebased this PR onto current Conflict-free branch: @reabr — happy to close this in favor of an update to your branch if you prefer authorship; otherwise this rebased branch is available for maintainers to cherry-pick/merge. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughA new ChangesCulture-aware metric scale word formatting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Humanizer/MetricNumeralExtensions.cs`:
- Around line 498-505: The code in MetricNumeralExtensions.cs currently checks
LongScaleCultures.Contains(culture.Name) OR
Contains(culture.TwoLetterISOLanguageName), which causes regional exceptions
like "pt-BR" to be masked by the language fallback; change the logic in the
branch that computes isLongScale (used when
formatValue.HasFlag(MetricNumeralFormats.UseScaleWord) and returning
UnitPrefixes[symbol].LongScaleWord/ShortScaleWord) to first check for an exact
culture.Name match and only if that fails fall back to checking the
TwoLetterISOLanguageName; apply the same exact fix to the second occurrence
around the later block (the other UseScaleWord branch referenced in the review).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 62af7691-caa9-4767-a688-0c38925829b5
📒 Files selected for processing (6)
src/Humanizer/MetricNumeralExtensions.cssrc/Humanizer/MetricNumeralFormats.cstests/Humanizer.Tests/ApiApprover/PublicApiApprovalTest.Approve_Public_Api.DotNet10_0.verified.txttests/Humanizer.Tests/ApiApprover/PublicApiApprovalTest.Approve_Public_Api.DotNet8_0.verified.txttests/Humanizer.Tests/ApiApprover/PublicApiApprovalTest.Approve_Public_Api.Net4_8.verified.txttests/Humanizer.Tests/MetricNumeralTests.cs
| if (formatValue.HasFlag(MetricNumeralFormats.UseScaleWord)) | ||
| { | ||
| var culture = CultureInfo.CurrentUICulture; | ||
| var isLongScale = LongScaleCultures.Contains(culture.Name) | ||
| || LongScaleCultures.Contains(culture.TwoLetterISOLanguageName); | ||
| return isLongScale | ||
| ? UnitPrefixes[symbol].LongScaleWord | ||
| : UnitPrefixes[symbol].ShortScaleWord; |
There was a problem hiding this comment.
Handle regional short-scale exceptions before the language fallback.
pt-BR still resolves to long scale here because culture.TwoLetterISOLanguageName is "pt", so the explicit “NOT pt-BR” exception in the set is defeated. That makes (1E9).ToMetric(...UseScaleWord) return the wrong word for Brazilian Portuguese.
💡 Suggested fix
+ static readonly HashSet<string> ShortScaleCultureOverrides =
+ [
+ "pt-BR"
+ ];
+
static string GetUnitText(char symbol, MetricNumeralFormats? formats)
{
if (formats.HasValue)
{
var formatValue = formats.Value;
@@
if (formatValue.HasFlag(MetricNumeralFormats.UseScaleWord))
{
var culture = CultureInfo.CurrentUICulture;
- var isLongScale = LongScaleCultures.Contains(culture.Name)
- || LongScaleCultures.Contains(culture.TwoLetterISOLanguageName);
+ var isLongScale = !ShortScaleCultureOverrides.Contains(culture.Name)
+ && (LongScaleCultures.Contains(culture.Name)
+ || LongScaleCultures.Contains(culture.TwoLetterISOLanguageName));
return isLongScale
? UnitPrefixes[symbol].LongScaleWord
: UnitPrefixes[symbol].ShortScaleWord;
}
}Also applies to: 558-565
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Humanizer/MetricNumeralExtensions.cs` around lines 498 - 505, The code in
MetricNumeralExtensions.cs currently checks
LongScaleCultures.Contains(culture.Name) OR
Contains(culture.TwoLetterISOLanguageName), which causes regional exceptions
like "pt-BR" to be masked by the language fallback; change the logic in the
branch that computes isLongScale (used when
formatValue.HasFlag(MetricNumeralFormats.UseScaleWord) and returning
UnitPrefixes[symbol].LongScaleWord/ShortScaleWord) to first check for an exact
culture.Name match and only if that fails fall back to checking the
TwoLetterISOLanguageName; apply the same exact fix to the second occurrence
around the later block (the other UseScaleWord branch referenced in the review).
|
@leno23 Appreciate the rebase work! Feel free to proceed with merging your rebased branch |
2 participants
Fixes #1675
What
Adds a new
MetricNumeralFormats.UseScaleWordflag that automaticallyselects between long and short scale words based on
CultureInfo.CurrentUICulture,as suggested in the issue.
Changes
UseScaleWord = 16flag toMetricNumeralFormatsenumLongScaleCultureslookup set inMetricNumeralExtensionscoveringGerman, French, Italian, Spanish, Portuguese, Dutch, Russian, Polish and Turkish
GetUnitTextto handle the new flag by checking the current UI cultureExample
(1E9).ToMetric(MetricNumeralFormats.WithSpace | MetricNumeralFormats.UseScaleWord)en-US→"1 billion"(short scale)de-DE→"1 milliard"(long scale)fr-FR→"1 milliard"(long scale)Tests
Added three test methods to
MetricNumeralTests.cscovering:[UseCulture])