fix: Accept format specifiers#42
Conversation
📝 WalkthroughWalkthroughThe PR updates named-placeholder handling in the Resx source generator, adds shared argument parsing and mixed-format detection, regenerates resource snapshots, and adds unit and integration tests for the new formatting behavior. ChangesNamed Placeholder Rewriting in ResxSourceGenerator
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![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: bd4263d584
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @"(?<!\{)\{([a-z]\w*)(?:,\s*-?\d+)?(?::[^{}]*)?\}(?!\})", | ||
| RegexOptions.IgnoreCase | RegexOptions.Compiled | ||
| ); | ||
| private static readonly Regex NumberParameterMatcher = new( | ||
| @"(?<!\{)\{(\d+)(?:,\s*-?\d+)?(?::[^{}]*)?\}(?!\})", |
There was a problem hiding this comment.
When a resource uses valid composite formats such as {{{0}}} or {{{name:T}}} to render literal braces around a formatted value, the actual format item is directly adjacent to the escaped brace pair. These negative lookbehind/ahead checks classify that item as escaped, so GetArguments returns no argument and the generator does not emit the expected Format... method; the generated named-argument replacer has the same adjacent-brace guard if another argument causes a method to be emitted. This regresses existing resources that format values while outputting literal braces.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/Darp.Utils.ResxSourceGenerator.Tests/ResxGeneratorTests.cs (1)
153-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one escaped-brace snapshot case here.
The new vectors cover bare
{arg[:format]}items, but not valid composites like{{{replacement:T}}}or{replacement}}}. A case like that would exercise the generatedReplaceNamedFormatItempath end-to-end and catch the brace-adjacent regressions above.🤖 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 `@test/Darp.Utils.ResxSourceGenerator.Tests/ResxGeneratorTests.cs` around lines 153 - 159, Add one escaped-brace snapshot case to ResxGeneratorTests so the generated ReplaceNamedFormatItem path is exercised with a brace-adjacent composite like {{{replacement:T}}} or {replacement}}}. Update the InlineData list in ResxGeneratorTests to include a representative escaped-brace input/output pair that verifies the generator handles valid composite formatting with adjacent braces.
🤖 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/Darp.Utils.ResxSourceGenerator/BuildHelper.cs`:
- Around line 153-154: The `ReplaceNamedFormatItem` logic in `BuildHelper` is
rejecting valid composite format matches whenever `suffixEnd + 1` is a `}`;
update that check so a doubled closing brace after a match is not treated as an
automatic skip. Adjust the validation around the named token replacement to
allow cases like `{name}}}` and `{{{name:T}}}` while still rejecting truly
invalid patterns, and keep the behavior localized to the match-processing path
that uses `suffixEnd`.
In `@src/Darp.Utils.ResxSourceGenerator/ResourceFormatHelper.cs`:
- Around line 10-16: The placeholder detection in ResourceFormatHelper’s
NamedParameterMatcher and NumberParameterMatcher is too strict because the brace
lookarounds reject valid composite format items adjacent to escaped braces.
Update GetArguments parsing to be brace-aware so it can recognize legal patterns
like {{{0:T}}} and {name}}} instead of relying on regex lookarounds that miss
them, and ensure the generator still emits format methods for those resource
strings.
In
`@test/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_0/Resources.Designer.g.cs`:
- Around line 106-107: The format-item detection in the resource-generated
string handling is incorrectly rejecting escaped closing braces, so named tokens
followed by a literal } remain unreplaced and can break the later string.Format
call. Update the matching logic around the brace scan in the generated formatter
helper so that the check in this block recognizes doubled }} as an escaped
literal brace instead of treating it as a non-match, while keeping the rest of
the token parsing in the same method intact.
---
Nitpick comments:
In `@test/Darp.Utils.ResxSourceGenerator.Tests/ResxGeneratorTests.cs`:
- Around line 153-159: Add one escaped-brace snapshot case to ResxGeneratorTests
so the generated ReplaceNamedFormatItem path is exercised with a brace-adjacent
composite like {{{replacement:T}}} or {replacement}}}. Update the InlineData
list in ResxGeneratorTests to include a representative escaped-brace
input/output pair that verifies the generator handles valid composite formatting
with adjacent braces.
🪄 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
Run ID: a056f59c-32c4-4eb4-8f16-1895d4b2ad11
📒 Files selected for processing (14)
Darp.Utils.slnxsrc/Darp.Utils.ResxSourceGenerator/BuildHelper.cssrc/Darp.Utils.ResxSourceGenerator/ResourceFormatHelper.cstest/Darp.Utils.ResxSourceGenerator.Integration/Darp.Utils.ResxSourceGenerator.Integration.csprojtest/Darp.Utils.ResxSourceGenerator.Integration/GeneratedResourcesTests.cstest/Darp.Utils.ResxSourceGenerator.Integration/Resources.resxtest/Darp.Utils.ResxSourceGenerator.Tests/ResourceFormatHelperTests.cstest/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_0/Resources.Designer.g.cstest/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_0_1_2/Resources.Designer.g.cstest/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_0_T/Resources.Designer.g.cstest/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_replacement/Resources.Designer.g.cstest/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_replacement_T/Resources.Designer.g.cstest/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_x/Resources.Designer.g.cstest/Darp.Utils.ResxSourceGenerator.Tests/ResxGeneratorTests.cs
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/Darp.Utils.ResxSourceGenerator/ResourceFormatHelper.cs`:
- Around line 36-42: The GetArguments logic in ResourceFormatHelper currently
returns namedArguments as soon as any named placeholder is present, which causes
mixed inputs like numbered plus named placeholders to lose the numeric slots.
Update GetArguments to either detect and reject mixed named/numbered
placeholders explicitly or to merge both sets safely, and make sure the
formatter generation path handles this consistently without collapsing
everything to a single named argument set.
🪄 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
Run ID: 3f26593c-12a2-4922-b958-7b073fdacd20
📒 Files selected for processing (13)
src/Darp.Utils.ResxSourceGenerator/BuildHelper.cssrc/Darp.Utils.ResxSourceGenerator/ResourceFormatHelper.cstest/Darp.Utils.ResxSourceGenerator.Integration/GeneratedResourcesTests.cstest/Darp.Utils.ResxSourceGenerator.Integration/Resources.resxtest/Darp.Utils.ResxSourceGenerator.Tests/ResourceFormatHelperTests.cstest/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_0/Resources.Designer.g.cstest/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_0_1_2/Resources.Designer.g.cstest/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_0_T/Resources.Designer.g.cstest/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_replacement/Resources.Designer.g.cstest/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_replacement_T/Resources.Designer.g.cstest/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_replacement_braces_T/Resources.Designer.g.cstest/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_x/Resources.Designer.g.cstest/Darp.Utils.ResxSourceGenerator.Tests/ResxGeneratorTests.cs
💤 Files with no reviewable changes (7)
- test/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_0_1_2/Resources.Designer.g.cs
- test/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_0_T/Resources.Designer.g.cs
- test/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_replacement/Resources.Designer.g.cs
- test/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_x/Resources.Designer.g.cs
- test/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_0/Resources.Designer.g.cs
- src/Darp.Utils.ResxSourceGenerator/BuildHelper.cs
- test/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_replacement_T/Resources.Designer.g.cs
✅ Files skipped from review due to trivial changes (2)
- test/Darp.Utils.ResxSourceGenerator.Integration/Resources.resx
- test/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_replacement_braces_T/Resources.Designer.g.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- test/Darp.Utils.ResxSourceGenerator.Integration/GeneratedResourcesTests.cs
- test/Darp.Utils.ResxSourceGenerator.Tests/ResxGeneratorTests.cs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb1ed772df
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return namedArguments; | ||
| } | ||
|
|
||
| return numberedArguments.OrderBy(Convert.ToInt32).ToList(); |
There was a problem hiding this comment.
When this new parser sees a valid composite format item whose first numeric index is not zero, e.g. {1:T}, it returns only "1"; the generated method then has one parameter and calls string.Format(Culture, @Name, p1), so the remaining {1:T} item always throws because argument slot 1 was never supplied. This newly affects the format-specifier cases being enabled here, so numeric arguments need to be expanded/mapped up to the maximum index rather than returning only the distinct indices found.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64482569f9
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| messageArgs: [name] | ||
| ) | ||
| ); | ||
| continue; |
There was a problem hiding this comment.
When a resource value mixes numbered and named items (for example {0} {name}) with EmitFormatMethods enabled, this continue exits the entire resource loop after the property has already been emitted but before the corresponding Keys constant is appended. The warning says only the format method should be omitted, but the generated property still references Keys.@..., so this case turns into a compilation error instead of a usable string property plus diagnostic.
Useful? React with 👍 / 👎.
| private static bool IsIdentifierStart(char c) => c is >= 'a' and <= 'z' or >= 'A' and <= 'Z'; | ||
|
|
||
| private static bool IsIdentifierPart(char c) => IsIdentifierStart(c) || c is (>= '0' and <= '9') or '_'; |
There was a problem hiding this comment.
For named placeholders that contain a valid non-ASCII C# identifier character after the first ASCII letter, such as {año} or {name_ä}, this ASCII-only check stops parsing at the first non-ASCII character and rejects the item as an invalid suffix, so no Format... method is generated. The previous matcher accepted these via \w*, and the project already has CharExtensions.IsIdentifierPartCharacter() for C#-compatible Unicode identifiers, so localized parameter names that used to work silently lose their format helper.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/Darp.Utils.ResxSourceGenerator.Tests/ResourceFormatHelperTests.cs (1)
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative-alignment coverage here.
This adds the
:formatcase, but the same parser path also accepts alignment with optional whitespace and-width. A regression there would still break valid composite formats, so please add cases like{0,-10}and{myName,-10:T}to this theory as well.
🤖 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/Darp.Utils.ResxSourceGenerator/BuildHelper.cs`:
- Around line 394-405: The mixed format arguments branch in BuildHelper should
not stop the rest of the resource generation flow after emitting
MixedFormatArgumentsWarning. Update the logic around
resourceString.HasMixedArguments so it only bypasses RenderFormatMethod, while
still allowing the Keys.* constant and property generation to proceed for that
resource. Keep the behavior localized to the mixed-argument handling in the
resource generation path.
In `@src/Darp.Utils.ResxSourceGenerator/ResourceFormatHelper.cs`:
- Around line 60-61: The numbered-argument handling in ResourceFormatHelper
should guard oversized format indexes before creating the contiguous list.
Update the logic that parses numberedArguments and builds the Enumerable.Range
result to use int.TryParse instead of Convert.ToInt32, reject or diagnose
indexes above a sane maximum, and skip generating the range when the parsed
value is too large. Keep the fix localized to the method that computes
maxArgumentIndex and the subsequent Enumerable.Range call.
🪄 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
Run ID: 959d94b4-debb-43e1-b099-f213e7155bcc
📒 Files selected for processing (8)
src/Darp.Utils.ResxSourceGenerator/AnalyzerReleases.Unshipped.mdsrc/Darp.Utils.ResxSourceGenerator/BuildHelper.cssrc/Darp.Utils.ResxSourceGenerator/ResourceFormatHelper.cstest/Darp.Utils.ResxSourceGenerator.Integration/GeneratedResourcesTests.cstest/Darp.Utils.ResxSourceGenerator.Integration/Resources.resxtest/Darp.Utils.ResxSourceGenerator.Tests/ResourceFormatHelperTests.cstest/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_1_T/Resources.Designer.g.cstest/Darp.Utils.ResxSourceGenerator.Tests/ResxGeneratorTests.cs
✅ Files skipped from review due to trivial changes (2)
- src/Darp.Utils.ResxSourceGenerator/AnalyzerReleases.Unshipped.md
- test/Darp.Utils.ResxSourceGenerator.Tests/Resources/SingleString_EmitFormatMethodsAsync_1_T/Resources.Designer.g.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- test/Darp.Utils.ResxSourceGenerator.Integration/GeneratedResourcesTests.cs
- test/Darp.Utils.ResxSourceGenerator.Integration/Resources.resx
- test/Darp.Utils.ResxSourceGenerator.Tests/ResxGeneratorTests.cs
| if (resourceString.HasMixedArguments) | ||
| { | ||
| diagnostics.Add( | ||
| Diagnostic.Create( | ||
| descriptor: MixedFormatArgumentsWarning, | ||
| location: Location.Create(resourceInformation.ResourceFile.Path, default, default), | ||
| messageArgs: [name] | ||
| ) | ||
| ); | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Don’t continue after reporting mixed format arguments.
This skips key generation for the resource after the property has already been emitted, so the generated property can reference a missing Keys.* constant. Only skip RenderFormatMethod.
Proposed fix
if (resourceString.HasMixedArguments)
{
diagnostics.Add(
Diagnostic.Create(
descriptor: MixedFormatArgumentsWarning,
location: Location.Create(resourceInformation.ResourceFile.Path, default, default),
messageArgs: [name]
)
);
- continue;
}
-
- RenderFormatMethod(memberIndent, membersBuilder, resourceString);
+ else
+ {
+ RenderFormatMethod(memberIndent, membersBuilder, resourceString);
+ }📝 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.
| if (resourceString.HasMixedArguments) | |
| { | |
| diagnostics.Add( | |
| Diagnostic.Create( | |
| descriptor: MixedFormatArgumentsWarning, | |
| location: Location.Create(resourceInformation.ResourceFile.Path, default, default), | |
| messageArgs: [name] | |
| ) | |
| ); | |
| continue; | |
| } | |
| if (resourceString.HasMixedArguments) | |
| { | |
| diagnostics.Add( | |
| Diagnostic.Create( | |
| descriptor: MixedFormatArgumentsWarning, | |
| location: Location.Create(resourceInformation.ResourceFile.Path, default, default), | |
| messageArgs: [name] | |
| ) | |
| ); | |
| } | |
| else | |
| { | |
| RenderFormatMethod(memberIndent, membersBuilder, resourceString); | |
| } |
🤖 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/Darp.Utils.ResxSourceGenerator/BuildHelper.cs` around lines 394 - 405,
The mixed format arguments branch in BuildHelper should not stop the rest of the
resource generation flow after emitting MixedFormatArgumentsWarning. Update the
logic around resourceString.HasMixedArguments so it only bypasses
RenderFormatMethod, while still allowing the Keys.* constant and property
generation to proceed for that resource. Keep the behavior localized to the
mixed-argument handling in the resource generation path.
| var maxArgumentIndex = numberedArguments.Select(x => Convert.ToInt32(x, CultureInfo.InvariantCulture)).Max(); | ||
| return Enumerable.Range(0, maxArgumentIndex + 1).Select(x => x.ToString(CultureInfo.InvariantCulture)).ToList(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard oversized numeric format indexes before building the contiguous range.
Convert.ToInt32 can throw for {999999999999}, and a valid-but-huge index like {100000000} can force an enormous generated parameter list. Add int.TryParse plus a sane upper bound/skip diagnostic before Enumerable.Range.
🤖 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/Darp.Utils.ResxSourceGenerator/ResourceFormatHelper.cs` around lines 60 -
61, The numbered-argument handling in ResourceFormatHelper should guard
oversized format indexes before creating the contiguous list. Update the logic
that parses numberedArguments and builds the Enumerable.Range result to use
int.TryParse instead of Convert.ToInt32, reject or diagnose indexes above a sane
maximum, and skip generating the range when the parsed value is too large. Keep
the fix localized to the method that computes maxArgumentIndex and the
subsequent Enumerable.Range call.
1 participant
Summary
Right now, the resx source generator detects numeric or named format parameters such as {0} or {my_name}.
However, when a format specifier is added, the generator does not emit a corresponding method.
Examples:
Changes
Testing
Added additional integration tests using the generated sources
Checklist