Fix Index Issue for Format String & Add Culture Info When Need to Format #17
Conversation
📝 WalkthroughWalkthroughThe update enhances the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (6)
✨ Finishing Touches
🪧 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 (5)
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs (5)
165-285: Consider edge cases in custom .NET format parsing.
This method correctly handles escaped braces and basic format items. However, more advanced format specifiers (e.g., alignment like "{0,5}" or nested braces) aren't currently handled here. If that's within scope, consider extending the logic to handle additional format scenarios.
612-614: Minor clarity suggestion for parameter gathering.
Storingls.Params.ToList()is fine, but you could consider usingvar parameters = ls.Params as IReadOnlyList<LocalizableStringParam> ?? ls.Params.ToList();if you frequently need a list. It’s a small readability improvement.
624-626: Avoid nested ternary for readability.
Using anifstatement or a local boolean variable to differentiate the presence of a custom format might make the code more readable than a nested ternary expression.
632-634: Consolidate similar logic for formatting.
As in lines 624-626, consider refactoring this nested ternary into a clearer conditional block. This will consistently improve maintainability and readability.
677-678: Clarify property name if usage is ambiguous.
Currently namedFormat, this property returns whether any parameter includes a format specifier. Consider a name likeHasCustomFormatorRequiresCultureFormattingif you want to convey intent more clearly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs (3)
156-158: Looks good – parameters are now parsed and updated.
Splitting the parameter extraction and comment-based enrichment steps into two separate calls improves maintainability and clarity.
287-329: Validate XML comment attributes before usage.
Accessing attributes likep.Attribute("index").Valuecan trigger exceptions if the XML comment is malformed or missing these attributes. Consider adding checks or graceful handling to avoid potential NullReferenceExceptions and improve resilience.
665-668: Adjustable parameter properties enhance flexibility.
Changing these fields to auto-properties with setters is a sensible move that allows for post-processing or updates to parameter definitions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs (1)
165-285: Robust implementation for format string parameter extraction.This new method thoroughly handles all format string scenarios including:
- Escaped braces (
{{and}})- Format specifiers (after
:)- Proper index parsing and validation
- Tracking of the maximum index to ensure no parameters are skipped
The implementation correctly addresses the index issue mentioned in the PR objectives by ensuring all parameters from index 0 to maxIndex are included, even if some indices are missing in the format string.
One minor optimization suggestion:
- var parameters = new Dictionary<int, string>(); + var parameters = new Dictionary<int, string>(capacity: format.Length / 10 + 1);Using a capacity hint can reduce allocations when dealing with longer format strings with many parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs(5 hunks)
🔇 Additional comments (6)
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs (6)
156-158: Good refactoring to improve parameter handling.The change from direct parameter parsing to using a dedicated
GetParametersmethod creates a cleaner separation of concerns. This is essential for the PR's goal of fixing the index issue for format strings.
293-324: Well-structured parameter update logic.The method now correctly checks for empty parameters and properly updates the provided parameter list, supporting the PR objectives. The parameter handling is comprehensive and handles XML parsing errors gracefully.
625-628: Critical culture-aware formatting implementation.This conditional formatting approach addresses the second objective of the PR by adding culture information when formatting strings. Using
CultureInfo.CurrentCultureensures numbers, dates, and other format-sensitive data will be displayed correctly according to the user's culture settings.The conditional check on
!ls.Formatis elegant and avoids unnecessary overhead when no specific formatting is needed.
634-637: Consistent cultural formatting for plugin translations.The same culture-aware formatting approach is correctly applied to plugin translations, ensuring consistent behavior across the application.
667-670: Property mutability enables better parameter handling.Changing these properties from get-only to mutable (
{ get; set; }) is necessary to support the updated parameter parsing and updating approach. The addition of theFormatproperty is essential for tracking formatting specifications.
679-680: Smart format detection with LINQ.This property efficiently determines if any parameter requires special formatting by checking if any parameter has a non-empty
Formatvalue. This drives the conditional formatting logic inGenerateLocalizationMethod.
|
With these translations: <!-- <param name="count" type="int" index="0" /> -->
<system:String x:Key="about_activate_times">You have activated Flow Launcher {0:D} times</system:String>
<!-- <param name="count" type="int" index="0" /> -->
<system:String x:Key="about_activate_times2">You have activated Flow Launcher {0,35} times</system:String>
<!-- <param name="count" type="int" index="0" /> -->
<system:String x:Key="about_activate_times3">You have activated Flow Launcher {0,35:D} times</system:String>It seems to generate this code for me: /// <code>
/// You have activated Flow Launcher {0:D} times
/// </code>
public static string about_activate_times(int count) => string.Format(System.Globalization.CultureInfo.CurrentCulture, Class1.Context.API.GetTranslation("about_activate_times"), count);
/// <code>
/// You have activated Flow Launcher {0,35} times
/// </code>
public static string about_activate_times2(int count) => string.Format(Class1.Context.API.GetTranslation("about_activate_times2"), count);
/// <code>
/// You have activated Flow Launcher {0,35:D} times
/// </code>
public static string about_activate_times3(int count) => string.Format(Class1.Context.API.GetTranslation("about_activate_times3"), count);I'm not sure about the second example, but I think the third one also should include culture? |
|
Well, let me check. iirc it can work for your third example. |
|
It seems that the previous version cannot fully support alignment string format like "{0,20:D}". |
|
For second example, I do not think we need to add culture info since alighment should be the same for all cultures. |
Fix Index Issue for Format String
If format string skips certain values, these missing data will not be generated. We should analyze format string first to get the max index.
Add Culture Info When Need to Format
When format string needs to be formatted (like
{1:D}), we should pass culture info so that it can parse string correctly.Test
The string resources are:
The generated codes are:
