Conversation
📝 WalkthroughWalkthroughAdds embedding of surrogate resource files to the project file, introduces a test that verifies all specific ICU cultures map without errors, updates tooling to generate/write per-culture surrogate .yml/.bin files and to materialize ICU-named surrogates from NLS equivalents, and adds a defensive null-check when loading embedded surrogates. Changes
Sequence DiagramsequenceDiagram
participant Tool as gse Tooling
participant FS as File System
participant Build as Build Process
participant Test as Test Runner
Tool->>Tool: Enumerate all specific cultures
Tool->>Tool: For each culture call WriteSurrogate(CultureInfo)
Tool->>FS: Write .yml and gzipped .bin surrogate files
Tool->>Tool: Call WriteIcuNamedNlsAlternatives()
Tool->>FS: TryWriteNlsSurrogate(NLS-name) or Copy NLS .yml/.bin to ICU-name
FS->>Build: Surrogate files are present on disk
Build->>Build: csproj embeds resources into assembly
Test->>Build: Test runner loads assembly
Test->>Test: UseNationalLanguageSupport_EnsureAllIcuCulturesAreMapped() validates mappings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Pull request overview
This pull request enhances the globalization extension library to support ICU-only culture names that don't have direct Windows NLS equivalents. It creates a mapping infrastructure that copies surrogate resource files from NLS culture names to their ICU counterparts, ensuring cross-platform compatibility.
Changes:
- Introduced a mapping dictionary and file-copying mechanism to create surrogate resources for 40 ICU culture names that differ from Windows NLS conventions
- Added test coverage to verify all ICU-specific cultures can be resolved using the
UseNationalLanguageSupport()extension method - Updated the project file to include 40 new embedded resource files (both removed from None and added as EmbeddedResource)
Reviewed changes
Copilot reviewed 3 out of 43 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tooling/gse/Program.cs | Adds WriteIcuNamedNlsAlternatives method with a dictionary mapping 40 ICU culture names to NLS equivalents, copies surrogate files for both .bin and .yml formats, and includes code formatting improvements |
| test/Codebelt.Extensions.Globalization.Tests/CultureInfoExtensionsTest.cs | Adds comprehensive test to verify all ICU cultures are mapped, includes code formatting improvements for ternary operator indentation |
| src/Codebelt.Extensions.Globalization/Codebelt.Extensions.Globalization.csproj | Removes 40 surrogate files from None and adds them as EmbeddedResource entries for ICU culture variants |
| 40 binary surrogate files (bgc-in.bin through zh-hant-tw.bin) | New binary resource files containing culture-specific formatting data for ICU culture names |
Comments suppressed due to low confidence (2)
test/Codebelt.Extensions.Globalization.Tests/CultureInfoExtensionsTest.cs:1306
- The test contains an empty foreach loop at lines 1303-1306. This appears to be dead code that serves no purpose. The loop iterates over missingIcuNames but doesn't perform any action inside the loop body. This should either be removed or the intended logic should be implemented (such as logging the missing culture names to test output).
test/Codebelt.Extensions.Globalization.Tests/CultureInfoExtensionsTest.cs:1298 - The exception variable is declared but never used. Consider either removing the variable declaration and using a catch block without a variable (catch (Exception)), or use the exception variable to provide more detailed test output (e.g., logging exception messages to help diagnose why certain cultures are failing).
![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
🧹 Nitpick comments (3)
tooling/gse/Program.cs (1)
123-131: Silent skip when source surrogate is missing could mask upstream failures.If the NLS surrogate
.binor.ymlwas never generated (e.g.,World.GetCulturesdidn't return a culture), the copy is silently skipped and the missing ICU surrogate will only surface later when the test or runtime fails. AConsole.WriteLinewarning here would make tooling runs easier to debug.Proposed fix
if (File.Exists(sourceBin)) { File.Copy(sourceBin, Path.Combine(SurrogatesPath, $"{icuName.ToLowerInvariant()}.bin"), overwrite: true); } + else + { + Console.WriteLine($"Warning: NLS source '{sourceBin}' not found for ICU name '{icuName}'."); + } if (File.Exists(sourceYml)) { File.Copy(sourceYml, Path.Combine(SurrogatesPathRaw, $"{icuName.ToLowerInvariant()}.yml"), overwrite: true); } + else + { + Console.WriteLine($"Warning: NLS source '{sourceYml}' not found for ICU name '{icuName}'."); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tooling/gse/Program.cs` around lines 123 - 131, The code currently silently skips copying ICU surrogate files when sourceBin or sourceYml don't exist; update the block around File.Exists checks (the logic that uses sourceBin, sourceYml, SurrogatesPath, SurrogatesPathRaw and icuName in Program.cs) to emit a Console.WriteLine warning whenever a source file is missing (include the expected source path and the target path in the message) before skipping the File.Copy, so missing surrogates are visible during tooling runs.test/Codebelt.Extensions.Globalization.Tests/CultureInfoExtensionsTest.cs (1)
1291-1298: Unused variablesutand swallowed exception details.
var suton line 1293 is assigned but never read — use a discard. Additionally, the caughtexceptionis unused; consider logging it for diagnostic visibility or at least including it in the failure output.Proposed fix
try { - var sut = culture.UseNationalLanguageSupport(); + _ = culture.UseNationalLanguageSupport(); } - catch (Exception exception) + catch (Exception) { missingIcuNames.Add(culture.Name); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Codebelt.Extensions.Globalization.Tests/CultureInfoExtensionsTest.cs` around lines 1291 - 1298, Replace the unused local and the swallowed exception: call UseNationalLanguageSupport with a discard (e.g., "_" instead of "var sut") and capture the caught exception details instead of ignoring them—append exception.ToString() (or exception.Message) to the diagnostic output (for example add to missingIcuNames or a new missingIcuDetails collection) or emit it via TestContext.WriteLine inside the catch; this change should be made around the try/catch that invokes UseNationalLanguageSupport and updates missingIcuNames.src/Codebelt.Extensions.Globalization/Codebelt.Extensions.Globalization.csproj (1)
12-1310: Consider using a wildcard glob to reduce maintenance burden.Both the original and new blocks explicitly enumerate every
.binsurrogate. A wildcard glob like<EmbeddedResource Include="Surrogates\*.bin" />(plus a single<None Remove="Surrogates\*.bin" />) would automatically pick up new surrogates produced by the tooling without manual csproj edits each time a culture is added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Codebelt.Extensions.Globalization/Codebelt.Extensions.Globalization.csproj` around lines 12 - 1310, Replace the long per-file lists with a single wildcard pair so the project automatically picks up new surrogate files: remove the repeated explicit <None Remove="Surrogates\*.bin" /> and <EmbeddedResource Include="Surrogates\*.bin" /> only (i.e. replace the many <None Remove="Surrogates\*.bin" /> entries with one <None Remove="Surrogates\*.bin" /> and replace the many <EmbeddedResource Include="Surrogates\*.bin" /> entries with one <EmbeddedResource Include="Surrogates\*.bin" />), keeping the <ItemGroup> wrapper(s) as needed so MSBuild still excludes then embeds all .bin files produced by tooling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/Codebelt.Extensions.Globalization.Tests/CultureInfoExtensionsTest.cs`:
- Around line 1301-1307: The empty foreach over missingIcuNames in
CultureInfoExtensionsTest is dead code; either remove the entire if/foreach
block or, if intended for diagnostics, iterate missingIcuNames and call
TestOutput.WriteLine for each item (e.g., "Missing ICU name: {name}") so the
test output shows which names are missing; update the block around
missingIcuNames accordingly.
---
Nitpick comments:
In
`@src/Codebelt.Extensions.Globalization/Codebelt.Extensions.Globalization.csproj`:
- Around line 12-1310: Replace the long per-file lists with a single wildcard
pair so the project automatically picks up new surrogate files: remove the
repeated explicit <None Remove="Surrogates\*.bin" /> and <EmbeddedResource
Include="Surrogates\*.bin" /> only (i.e. replace the many <None
Remove="Surrogates\*.bin" /> entries with one <None Remove="Surrogates\*.bin" />
and replace the many <EmbeddedResource Include="Surrogates\*.bin" /> entries
with one <EmbeddedResource Include="Surrogates\*.bin" />), keeping the
<ItemGroup> wrapper(s) as needed so MSBuild still excludes then embeds all .bin
files produced by tooling.
In `@test/Codebelt.Extensions.Globalization.Tests/CultureInfoExtensionsTest.cs`:
- Around line 1291-1298: Replace the unused local and the swallowed exception:
call UseNationalLanguageSupport with a discard (e.g., "_" instead of "var sut")
and capture the caught exception details instead of ignoring them—append
exception.ToString() (or exception.Message) to the diagnostic output (for
example add to missingIcuNames or a new missingIcuDetails collection) or emit it
via TestContext.WriteLine inside the catch; this change should be made around
the try/catch that invokes UseNationalLanguageSupport and updates
missingIcuNames.
In `@tooling/gse/Program.cs`:
- Around line 123-131: The code currently silently skips copying ICU surrogate
files when sourceBin or sourceYml don't exist; update the block around
File.Exists checks (the logic that uses sourceBin, sourceYml, SurrogatesPath,
SurrogatesPathRaw and icuName in Program.cs) to emit a Console.WriteLine warning
whenever a source file is missing (include the expected source path and the
target path in the message) before skipping the File.Copy, so missing surrogates
are visible during tooling runs.
Comment on lines
+1301
to
+1307
| if (missingIcuNames.Count > 0) | ||
| { | ||
| foreach (var missingIcuName in missingIcuNames) | ||
| { | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
Empty foreach loop — dead code or incomplete implementation.
The if block on lines 1301-1307 iterates missingIcuNames but the loop body is empty. This looks like leftover scaffolding — likely intended to log each missing name via TestOutput.WriteLine.
Proposed fix: either remove the block or log the missing names
- if (missingIcuNames.Count > 0)
- {
- foreach (var missingIcuName in missingIcuNames)
- {
-
- }
- }
+ foreach (var missingIcuName in missingIcuNames)
+ {
+ TestOutput.WriteLine($"Missing ICU mapping: {missingIcuName}");
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/Codebelt.Extensions.Globalization.Tests/CultureInfoExtensionsTest.cs`
around lines 1301 - 1307, The empty foreach over missingIcuNames in
CultureInfoExtensionsTest is dead code; either remove the entire if/foreach
block or, if intended for diagnostics, iterate missingIcuNames and call
TestOutput.WriteLine for each item (e.g., "Missing ICU name: {name}") so the
test output shows which names are missing; update the block around
missingIcuNames accordingly.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tooling/gse/Program.cs (2)
176-194: Silent skip when source surrogate files are missing.If
sourceBin/sourceYmldon't exist, the loop silently moves on. For a build-time code-generation tool, this can mask a misconfigured mapping (e.g., a typo in the NLS name, or the NLS surrogate never having been written in the earlier loop). AConsole.WriteLinewarning would surface these issues during generation without breaking the flow.♻️ Suggested improvement
if (File.Exists(sourceBin)) { File.Copy(sourceBin, Path.Combine(SurrogatesPath, $"{icuName.ToLowerInvariant()}.bin"), overwrite: true); } + else + { + Console.WriteLine($"Warning: NLS source '{sourceBin}' not found for ICU mapping '{icuName}'."); + } if (File.Exists(sourceYml)) { File.Copy(sourceYml, Path.Combine(SurrogatesPathRaw, $"{icuName.ToLowerInvariant()}.yml"), overwrite: true); } + else + { + Console.WriteLine($"Warning: NLS raw source '{sourceYml}' not found for ICU mapping '{icuName}'."); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tooling/gse/Program.cs` around lines 176 - 194, The loop that copies mapped surrogate files (inside the foreach over icuToNls) silently skips when sourceBin or sourceYml are missing; update the logic in the foreach block (around TryWriteNlsSurrogate, sourceBin, sourceYml and the File.Exists checks) to emit a Console.WriteLine warning identifying the missing file(s) and the affected mapping (icuName -> nlsName) whenever neither the .bin nor .yml source exists so build-time misconfigurations are visible while keeping the existing copy behavior otherwise.
43-66:msstream fromYamlFormatter.SerializeObjectis never disposed.While
MemoryStream.Dispose()is largely a no-op in .NET, wrappingmsin ausingstatement keeps the method consistent with howfsRawYaml,cms, andfsare handled.♻️ Suggested fix
private static void WriteSurrogate(CultureInfo cultureInfo) { var dtSurrogate = new DateTimeFormatInfoSurrogate(cultureInfo.DateTimeFormat); var nfSurrogate = new NumberFormatInfoSurrogate(cultureInfo.NumberFormat); var ciSurrogate = new CultureInfoSurrogate(dtSurrogate, nfSurrogate); - var ms = YamlFormatter.SerializeObject(ciSurrogate, o => + using var ms = YamlFormatter.SerializeObject(ciSurrogate, o => { o.Settings.NamingConvention = NullNamingConvention.Instance; o.Settings.ReflectionRules = new MemberReflection(); o.Settings.IndentSequences = false; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tooling/gse/Program.cs` around lines 43 - 66, The MemoryStream returned by YamlFormatter.SerializeObject (variable ms in WriteSurrogate) is not disposed; change its declaration to a using declaration (e.g. using var ms = YamlFormatter.SerializeObject(...)) so ms is disposed after you finish creating fsRawYaml, cms and fs; keep the existing sequence (create ms, write raw YAML, create cms = ms.CompressGZip(), write compressed blob) and preserve the LeaveOpen behavior on the ToByteArray calls so disposing ms happens only after you finish using it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tooling/gse/Program.cs`:
- Around line 176-194: The loop that copies mapped surrogate files (inside the
foreach over icuToNls) silently skips when sourceBin or sourceYml are missing;
update the logic in the foreach block (around TryWriteNlsSurrogate, sourceBin,
sourceYml and the File.Exists checks) to emit a Console.WriteLine warning
identifying the missing file(s) and the affected mapping (icuName -> nlsName)
whenever neither the .bin nor .yml source exists so build-time misconfigurations
are visible while keeping the existing copy behavior otherwise.
- Around line 43-66: The MemoryStream returned by YamlFormatter.SerializeObject
(variable ms in WriteSurrogate) is not disposed; change its declaration to a
using declaration (e.g. using var ms = YamlFormatter.SerializeObject(...)) so ms
is disposed after you finish creating fsRawYaml, cms and fs; keep the existing
sequence (create ms, write raw YAML, create cms = ms.CompressGZip(), write
compressed blob) and preserve the LeaveOpen behavior on the ToByteArray calls so
disposing ms happens only after you finish using it.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Codebelt.Extensions.Globalization/CultureInfoExtensions.cs (1)
29-31: 🛠️ Refactor suggestion | 🟠 MajorDocument the new
InvalidOperationExceptionin the XML doc for both public overloads.Neither overload's
<exception>block covers theInvalidOperationExceptionnow thrown at line 63 when a culture has no embedded surrogate. The collection overload throws it directly; the single-culture overload inherits the same path via delegation.📄 Proposed additions
/// <exception cref="ArgumentNullException"> /// <paramref name="culture"/> is null. /// </exception> +/// <exception cref="InvalidOperationException"> +/// No NLS surrogate is available for <paramref name="culture"/>. +/// </exception> public static CultureInfo UseNationalLanguageSupport(this CultureInfo culture)/// <exception cref="ArgumentNullException"> /// <paramref name="cultures"/> is null. /// </exception> +/// <exception cref="InvalidOperationException"> +/// No NLS surrogate is available for one of the elements in <paramref name="cultures"/>. +/// </exception> public static IEnumerable<CultureInfo> UseNationalLanguageSupport(this IEnumerable<CultureInfo> cultures)As per coding guidelines: "Add XML doc comments to public and protected classes and methods where appropriate."
Also applies to: 44-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Codebelt.Extensions.Globalization/CultureInfoExtensions.cs` around lines 29 - 31, Update the XML docs for the two public overloads in CultureInfoExtensions (the single-culture overload taking CultureInfo and the collection overload taking IEnumerable<CultureInfo> or similar) to include an <exception cref="InvalidOperationException"> entry explaining it is thrown when a culture (or any culture in the collection) has no embedded surrogate; ensure both overloads’ <exception> blocks document this new InvalidOperationException alongside the existing ArgumentNullException notes.
🧹 Nitpick comments (1)
tooling/gse/Program.cs (1)
55-62: Preferbytes.Lengthover(int)stream.Lengthas theWritecount argument.Both
ms.ToByteArray(...)andcms.ToByteArray(...)return the serialized/compressed data. Passing(int)ms.Length/(int)cms.Lengthas thecountimplicitly assumes those lengths match the returned array, which holds today because the CuemonToByteArrayextension usesMemoryStream.ToArray()(returning exactlyLengthbytes) forMemoryStreaminstances. Using the returned array's own.Lengthmakes the intent explicit and is more defensive if the stream type ever changes.♻️ Proposed refactor
- var rawYamlBytes = ms.ToByteArray(o => o.LeaveOpen = true); - fsRawYaml.Write(rawYamlBytes, 0, (int)ms.Length); + var rawYamlBytes = ms.ToByteArray(o => o.LeaveOpen = true); + fsRawYaml.Write(rawYamlBytes, 0, rawYamlBytes.Length); fsRawYaml.Flush(); using var cms = ms.CompressGZip(); - var compressedBytes = cms.ToByteArray(o => o.LeaveOpen = true); - fs.Write(compressedBytes, 0, (int)cms.Length); + var compressedBytes = cms.ToByteArray(o => o.LeaveOpen = true); + fs.Write(compressedBytes, 0, compressedBytes.Length); fs.Flush();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tooling/gse/Program.cs` around lines 55 - 62, The Write calls currently use (int)ms.Length/(int)cms.Length which assumes the stream lengths match the returned arrays; instead capture the byte arrays returned by ms.ToByteArray(...) and cms.ToByteArray(...) (e.g., var raw = ms.ToByteArray(...); var compressed = cms.ToByteArray(...)) and pass raw.Length and compressed.Length as the count to fsRawYaml.Write and fs.Write respectively (use the returned arrays' Length to be explicit and defensive when writing from fsRawYaml/fs), keeping the existing LeaveOpen options when calling ToByteArray.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tooling/gse/Program.cs`:
- Around line 173-191: The loop over icuToNls silently skips copying when
TryWriteNlsSurrogate(icuName) returns false and the mapped files
(sourceBin/sourceYml under SurrogatesPath/SurrogatesPathRaw) are missing; add
Console.Error warnings when File.Exists(sourceBin) or File.Exists(sourceYml) is
false to surface stale/missing mappings at generation time. Specifically, in the
foreach handling of (icuName, nlsName) around TryWriteNlsSurrogate, after
computing sourceBin and sourceYml and before/after each File.Copy, emit a
descriptive Console.Error message including icuName and nlsName whenever a
required source file is not found so tooling users see which ICU cultures will
lack surrogates (this will help trace later InvalidOperationException from
UseNationalLanguageSupport).
---
Outside diff comments:
In `@src/Codebelt.Extensions.Globalization/CultureInfoExtensions.cs`:
- Around line 29-31: Update the XML docs for the two public overloads in
CultureInfoExtensions (the single-culture overload taking CultureInfo and the
collection overload taking IEnumerable<CultureInfo> or similar) to include an
<exception cref="InvalidOperationException"> entry explaining it is thrown when
a culture (or any culture in the collection) has no embedded surrogate; ensure
both overloads’ <exception> blocks document this new InvalidOperationException
alongside the existing ArgumentNullException notes.
---
Nitpick comments:
In `@tooling/gse/Program.cs`:
- Around line 55-62: The Write calls currently use
(int)ms.Length/(int)cms.Length which assumes the stream lengths match the
returned arrays; instead capture the byte arrays returned by ms.ToByteArray(...)
and cms.ToByteArray(...) (e.g., var raw = ms.ToByteArray(...); var compressed =
cms.ToByteArray(...)) and pass raw.Length and compressed.Length as the count to
fsRawYaml.Write and fs.Write respectively (use the returned arrays' Length to be
explicit and defensive when writing from fsRawYaml/fs), keeping the existing
LeaveOpen options when calling ToByteArray.
Comment on lines
+173
to
191
| foreach (var (icuName, nlsName) in icuToNls) | ||
| { | ||
| // Prefer a native NLS surrogate for the ICU name if Windows supports it; | ||
| // only fall back to copying the mapped NLS surrogate if it does not. | ||
| if (TryWriteNlsSurrogate(icuName)) continue; | ||
|
|
||
| var sourceBin = Path.Combine(SurrogatesPath, $"{nlsName.ToLowerInvariant()}.bin"); | ||
| var sourceYml = Path.Combine(SurrogatesPathRaw, $"{nlsName.ToLowerInvariant()}.yml"); | ||
|
|
||
| if (File.Exists(sourceBin)) | ||
| { | ||
| File.Copy(sourceBin, Path.Combine(SurrogatesPath, $"{icuName.ToLowerInvariant()}.bin"), overwrite: true); | ||
| } | ||
|
|
||
| if (File.Exists(sourceYml)) | ||
| { | ||
| var dtSurrogate = new DateTimeFormatInfoSurrogate(cultureInfo.DateTimeFormat); | ||
| var nfSurrogate = new NumberFormatInfoSurrogate(cultureInfo.NumberFormat); | ||
| var ciSurrogate = new CultureInfoSurrogate(dtSurrogate, nfSurrogate); | ||
|
|
||
| var ms = YamlFormatter.SerializeObject(ciSurrogate, o => | ||
| { | ||
| o.Settings.NamingConvention = NullNamingConvention.Instance; | ||
| o.Settings.ReflectionRules = new MemberReflection(); | ||
| o.Settings.IndentSequences = false; | ||
| }); | ||
|
|
||
| using var fsRawYaml = new FileStream(Path.Combine(SurrogatesPathRaw, $"{cultureInfo.Name.ToLowerInvariant()}.yml"), FileMode.Create); | ||
| fsRawYaml.Write(ms.ToByteArray(o => o.LeaveOpen = true), 0, (int)ms.Length); | ||
| fsRawYaml.Flush(); | ||
|
|
||
| using var cms = ms.CompressGZip(); | ||
| using var fs = new FileStream(Path.Combine(SurrogatesPath, $"{cultureInfo.Name.ToLowerInvariant()}.bin"), FileMode.Create); | ||
| fs.Write(cms.ToByteArray(o => o.LeaveOpen = true), 0, (int)cms.Length); | ||
| fs.Flush(); | ||
| File.Copy(sourceYml, Path.Combine(SurrogatesPathRaw, $"{icuName.ToLowerInvariant()}.yml"), overwrite: true); | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent skip when source surrogate files are missing can produce an invisible mapping gap.
When TryWriteNlsSurrogate returns false and either sourceBin or sourceYml doesn't exist (e.g., the NLS name in the dictionary is stale or was never generated), the ICU culture is silently left without a surrogate. At runtime, UseNationalLanguageSupport will throw InvalidOperationException for that culture with no indication that the tooling already knew about the gap. Emitting at least a Console.Error warning when a copy is skipped would surface the problem at generation time.
🛡️ Proposed improvement
if (File.Exists(sourceBin))
{
File.Copy(sourceBin, Path.Combine(SurrogatesPath, $"{icuName.ToLowerInvariant()}.bin"), overwrite: true);
}
+ else
+ {
+ Console.Error.WriteLine($"[WARN] No surrogate found for ICU culture '{icuName}': source '{sourceBin}' does not exist. This culture will have no embedded resource.");
+ }
if (File.Exists(sourceYml))
{
File.Copy(sourceYml, Path.Combine(SurrogatesPathRaw, $"{icuName.ToLowerInvariant()}.yml"), overwrite: true);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| foreach (var (icuName, nlsName) in icuToNls) | |
| { | |
| // Prefer a native NLS surrogate for the ICU name if Windows supports it; | |
| // only fall back to copying the mapped NLS surrogate if it does not. | |
| if (TryWriteNlsSurrogate(icuName)) continue; | |
| var sourceBin = Path.Combine(SurrogatesPath, $"{nlsName.ToLowerInvariant()}.bin"); | |
| var sourceYml = Path.Combine(SurrogatesPathRaw, $"{nlsName.ToLowerInvariant()}.yml"); | |
| if (File.Exists(sourceBin)) | |
| { | |
| File.Copy(sourceBin, Path.Combine(SurrogatesPath, $"{icuName.ToLowerInvariant()}.bin"), overwrite: true); | |
| } | |
| if (File.Exists(sourceYml)) | |
| { | |
| var dtSurrogate = new DateTimeFormatInfoSurrogate(cultureInfo.DateTimeFormat); | |
| var nfSurrogate = new NumberFormatInfoSurrogate(cultureInfo.NumberFormat); | |
| var ciSurrogate = new CultureInfoSurrogate(dtSurrogate, nfSurrogate); | |
| var ms = YamlFormatter.SerializeObject(ciSurrogate, o => | |
| { | |
| o.Settings.NamingConvention = NullNamingConvention.Instance; | |
| o.Settings.ReflectionRules = new MemberReflection(); | |
| o.Settings.IndentSequences = false; | |
| }); | |
| using var fsRawYaml = new FileStream(Path.Combine(SurrogatesPathRaw, $"{cultureInfo.Name.ToLowerInvariant()}.yml"), FileMode.Create); | |
| fsRawYaml.Write(ms.ToByteArray(o => o.LeaveOpen = true), 0, (int)ms.Length); | |
| fsRawYaml.Flush(); | |
| using var cms = ms.CompressGZip(); | |
| using var fs = new FileStream(Path.Combine(SurrogatesPath, $"{cultureInfo.Name.ToLowerInvariant()}.bin"), FileMode.Create); | |
| fs.Write(cms.ToByteArray(o => o.LeaveOpen = true), 0, (int)cms.Length); | |
| fs.Flush(); | |
| File.Copy(sourceYml, Path.Combine(SurrogatesPathRaw, $"{icuName.ToLowerInvariant()}.yml"), overwrite: true); | |
| } | |
| } | |
| foreach (var (icuName, nlsName) in icuToNls) | |
| { | |
| // Prefer a native NLS surrogate for the ICU name if Windows supports it; | |
| // only fall back to copying the mapped NLS surrogate if it does not. | |
| if (TryWriteNlsSurrogate(icuName)) continue; | |
| var sourceBin = Path.Combine(SurrogatesPath, $"{nlsName.ToLowerInvariant()}.bin"); | |
| var sourceYml = Path.Combine(SurrogatesPathRaw, $"{nlsName.ToLowerInvariant()}.yml"); | |
| if (File.Exists(sourceBin)) | |
| { | |
| File.Copy(sourceBin, Path.Combine(SurrogatesPath, $"{icuName.ToLowerInvariant()}.bin"), overwrite: true); | |
| } | |
| else | |
| { | |
| Console.Error.WriteLine($"[WARN] No surrogate found for ICU culture '{icuName}': source '{sourceBin}' does not exist. This culture will have no embedded resource."); | |
| } | |
| if (File.Exists(sourceYml)) | |
| { | |
| File.Copy(sourceYml, Path.Combine(SurrogatesPathRaw, $"{icuName.ToLowerInvariant()}.yml"), overwrite: true); | |
| } | |
| else | |
| { | |
| Console.Error.WriteLine($"[WARN] No surrogate found for ICU culture '{icuName}': source '{sourceYml}' does not exist. This culture will have no embedded resource."); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tooling/gse/Program.cs` around lines 173 - 191, The loop over icuToNls
silently skips copying when TryWriteNlsSurrogate(icuName) returns false and the
mapped files (sourceBin/sourceYml under SurrogatesPath/SurrogatesPathRaw) are
missing; add Console.Error warnings when File.Exists(sourceBin) or
File.Exists(sourceYml) is false to surface stale/missing mappings at generation
time. Specifically, in the foreach handling of (icuName, nlsName) around
TryWriteNlsSurrogate, after computing sourceBin and sourceYml and before/after
each File.Copy, emit a descriptive Console.Error message including icuName and
nlsName whenever a required source file is not found so tooling users see which
ICU cultures will lack surrogates (this will help trace later
InvalidOperationException from UseNationalLanguageSupport).
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces improvements to handling ICU-only culture names in the globalization extension project. The main focus is on ensuring that surrogate resources for these cultures are included and mapped correctly, both in the build process and in test coverage. Additionally, code formatting and organization have been improved in several files.
Resource handling and mapping enhancements:
Codebelt.Extensions.Globalization.csproj)WriteIcuNamedNlsAlternatives, inProgram.csto map ICU-only culture names to their nearest NLS equivalents and copy surrogate files accordingly, so ICU-based environments can resolve them from the same embedded resource set. (tooling/gse/Program.cs)Testing improvements:
UseNationalLanguageSupport_EnsureAllIcuCulturesAreMapped, to verify that all ICU culture names are mapped and covered by surrogate resources, improving test coverage for culture support. (CultureInfoExtensionsTest.cs)Code organization and formatting:
usingstatements for clarity. (CultureInfoExtensionsTest.cs,Program.cs) [1] [2] [3]Summary by CodeRabbit
Tests
Chores
Bug Fixes