Rewrite to IIncrementalGenerator and update NuGet package #6
Conversation
📝 WalkthroughWalkthroughThis pull request updates package dependency versions in the Localization projects and refactors the source generator for localization. The dependency updates in the project files upgrade various Microsoft.CodeAnalysis packages. The Changes
Sequence Diagram(s)sequenceDiagram
participant IDE as Compiler/IDE
participant Gen as LocalizeSourceGenerator
participant Ctx as IncrementalGeneratorInitializationContext
participant Prov as AdditionalText Provider
participant Parser as XAML Parser
participant Output as SourceOutput
IDE->>Gen: Initialize(context)
Gen->>Ctx: Register resource & syntax providers
Ctx->>Prov: Provide additional texts
Gen->>Parser: Parse XAML files for localization data
Parser-->>Gen: Return localized strings, keys, and plugin info
Gen->>Output: Generate source code and report diagnostics
Output-->>IDE: Compiled output with generated code
Possibly related PRs
Suggested reviewers
Poem
✨ 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: 1
🧹 Nitpick comments (5)
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs (5)
124-161: Potential whitespace or newline comment issue.
Relying onelement.PreviousNodeto detect an associatedXCommentcan fail if there's a whitespace or formatting node in between. Consider ignoring non-comment whitespace nodes or searching preceding siblings to ensure comments are correctly associated.
241-326: Plugin detection logic is robust.
It correctly identifies classes implementingIPluginI18nand checks for a valid context property. If multiple classes implement IPluginI18n, you may wish to handle them or report a conflict.
337-435: Consider re-adding the logic to skip generating methods for unused keys.
Currently, you diagnose unused keys, but still generate methods for them. You have commented-out code that would skip method generation if the key is unused. Reintroducing that might reduce clutter in the final source.Example snippet demonstrating the reintroduction:
foreach (var ls in localizedStrings) { - // ... - /*if (unusedKeys.Contains(ls.Key)) - { - continue; - }*/ GenerateDocComments(sourceBuilder, ls, tabString); GenerateLocalizationMethod(sourceBuilder, ls, isCoreAssembly, pluginInfo, tabString); }
507-523: Only parsing placeholders up to {9}.
This loop stops at 9. Any placeholders above {9} are ignored. If you ever need more than ten placeholders, consider a dynamic approach rather than a fixed limit.
525-537: Stackalloc usage is a minor micro-optimization.
You could simplify it usingnew string(' ', n * 4)for readability.-private static string Spacing(int n) -{ - Span<char> spaces = stackalloc char[n * 4]; - spaces.Fill(' '); - var sb = new StringBuilder(n * 4); - foreach (var c in spaces) - { - _ = sb.Append(c); - } - return sb.ToString(); -} +private static string Spacing(int n) +{ + return new string(' ', n * 4); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Localization.Analyzers/Flow.Launcher.Localization.Analyzers.csproj(1 hunks)Flow.Launcher.Localization.SourceGenerators/Flow.Launcher.Localization.SourceGenerators.csproj(1 hunks)Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs(2 hunks)
🔇 Additional comments (20)
Flow.Launcher.Localization.SourceGenerators/Flow.Launcher.Localization.SourceGenerators.csproj (2)
11-14: Update PackageReference for Microsoft.CodeAnalysis.Analyzers
The package version has been successfully bumped to 3.11.0, which should provide improved diagnostics and analysis features. Please ensure that these analyzer enhancements are compatible with your current codebase and that no unexpected warnings or errors appear.
15-15: Upgrade PackageReference for Microsoft.CodeAnalysis.CSharp
The updated version 4.13.0 will help leverage the latest Roslyn compiler features and bug fixes. Verify that the changes made to the source generator (i.e., refactoring toIIncrementalGenerator) remain fully compatible with this new version of the C# compiler.Flow.Launcher.Localization.Analyzers/Flow.Launcher.Localization.Analyzers.csproj (3)
11-14: Update PackageReference for Microsoft.CodeAnalysis.Analyzers
The version has been updated from a lower version (previously 3.3.4) to 3.11.0. This upgrade helps align with recent improvements and bug fixes. Please verify that there are no breaking changes or new analyzers warnings introduced by this version change.
15-15: Update PackageReference for Microsoft.CodeAnalysis.CSharp
The dependency has been upgraded to version 4.13.0 (from 4.9.2). This update should facilitate compatibility with the incremental generator API implementation and other Roslyn-based improvements.
16-16: Update PackageReference for Microsoft.CodeAnalysis.CSharp.Workspaces
The version has been updated to 4.13.0. Ensure that any workspace-related APIs used in conjunction with the new IIncrementalGenerator implementation remain compatible with this upgrade.Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs (15)
3-3: No concerns on the new import.
This import ofSystem.Collections.Immutableis necessary for handling immutable collections in incremental generation.
7-7: No concerns on the new import.
TheSystem.Threadingusage for cancellation tokens is standard within incremental generators.
16-18: Doc comment is well-structured.
This comment block clearly conveys the purpose of the class and its responsibilities.
20-20: Switch to IIncrementalGenerator looks correct.
This aligns with modern Roslyn incremental generation patterns and helps improve performance by only regenerating code when necessary.
22-22: Region usage is fine.
Helps in structuring the code.
35-38: No issues with the package version retrieval.
Storing and using the assembly version can help track generator versions in generated code.
39-39: Region declaration is consistent.
No further remarks.
78-120: Initialization flow is clear and well-structured.
Diagnosing a missing resource dictionary is handled properly.
163-167: Implementation looks good.
Straightforward extraction of localizable string data.
169-192: No issues in parsing the XML comment content.
Parsing<summary>and<param>elements from XML within a comment is neatly handled.
198-238: User invocation key extraction is correct.
The approach to parse member access expressions and reverse them for the check is valid.
328-331: Location helper method appears correct.
No issues retrieving location objects for diagnostics.
451-469: Documentation generation is thorough.
Including both summary and<code>sections provides clarity for the generated methods.
471-505: Method generation logic is correct.
Handles both theCoreassembly and plugin assemblies gracefully, with a fallback for invalid scenarios.
541-609: Class definitions appear logical and consistent.
The data structures for method parameters, localizable strings, and plugin info are well-structured.
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs
Show resolved
Hide resolved
|
@Yusyuriv Hi, if you have time, could you test the program? Please don't forget to change the version of the NuGet package (I've found that for VS, the analyzers for the same NuGet package seems to be cached which causes the new codes not to be loaded) One more thing, it's the fourth point in my description above, I would like us to disable this optimization feature because it may cause some strings to be missing in the generated code. And both your and my codes cannot work. What do you think of this? Or do you have any ideas to fix these codes? |
| } | ||
|
|
||
| private static bool DoesClassImplementInterface(ClassDeclarationSyntax classDeclaration, string interfaceName) | ||
| private static string Spacing(int n) |
There was a problem hiding this comment.
Wouldn't this work?
private static string Spacing(int n)
{
return new string(' ', n * 4);
}There was a problem hiding this comment.
I think so.
I get these codes from https://github.com/files-community/Files and they should have their reason not to use this.
So I think it would better not to change them.
|
@Jack251970 It seemed to start working again as soon as I replaced the version numbers with 1.0.0. |
For me, I must change the NuGet version to make lates codes work. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs (3)
83-88: Consider creating a data container class for the complex parameter type.The current parameter type for the
Executemethod is complex and difficult to read. Consider extracting it to a dedicated class to improve readability.- private void Execute(SourceProductionContext spc, - ((((ImmutableArray<LocalizableString> LocalizableStrings, - ImmutableHashSet<string> InvocationKeys), - ImmutableArray<PluginClassInfo> PluginClassInfos), - Compilation Compilation), - ImmutableArray<AdditionalText> AdditionalTexts) data) + private void Execute(SourceProductionContext spc, GenerationData data) + + // Define this class outside the method + private class GenerationData + { + public ImmutableArray<LocalizableString> LocalizableStrings { get; init; } + public ImmutableHashSet<string> InvocationKeys { get; init; } + public ImmutableArray<PluginClassInfo> PluginClassInfos { get; init; } + public Compilation Compilation { get; init; } + public ImmutableArray<AdditionalText> AdditionalTexts { get; init; } + }
383-406: Clean up commented-out debugging code.Consider either fully removing the debugging code or placing it behind a compiler directive to keep the production code clean.
- // Uncomment them for debugging - //sourceBuilder.AppendLine("/*"); - /*// Generate all localization strings - sourceBuilder.AppendLine("localizedStrings"); - foreach (var ls in localizedStrings) - { - sourceBuilder.AppendLine($"{ls.Key} - {ls.Value}"); - } - sourceBuilder.AppendLine(); - - // Generate all unused keys - sourceBuilder.AppendLine("unusedKeys"); - foreach (var key in unusedKeys) - { - sourceBuilder.AppendLine($"{key}"); - } - sourceBuilder.AppendLine(); - - // Generate all used keys - sourceBuilder.AppendLine("usedKeys"); - foreach (var key in usedKeys) - { - sourceBuilder.AppendLine($"{key}"); - }*/ - //sourceBuilder.AppendLine("*/"); + #if DEBUG + // Debug-only code to output all keys + sourceBuilder.AppendLine("/*"); + sourceBuilder.AppendLine("localizedStrings:"); + foreach (var ls in localizedStrings) + { + sourceBuilder.AppendLine($"{ls.Key} - {ls.Value}"); + } + + sourceBuilder.AppendLine("unusedKeys:"); + foreach (var key in unusedKeys) + { + sourceBuilder.AppendLine($"{key}"); + } + + sourceBuilder.AppendLine("usedKeys:"); + foreach (var key in usedKeys) + { + sourceBuilder.AppendLine($"{key}"); + } + sourceBuilder.AppendLine("*/"); + #endif
521-533: Consider simplifying the Spacing method.The current implementation uses low-level constructs like
Span<char>and manualStringBuilderoperations, which increases complexity for a simple operation.private static string Spacing(int n) { - Span<char> spaces = stackalloc char[n * 4]; - spaces.Fill(' '); - - var sb = new StringBuilder(n * 4); - foreach (var c in spaces) - { - _ = sb.Append(c); - } - - return sb.ToString(); + return new string(' ', n * 4); }This simplification was suggested in a past comment and achieves the same result with much simpler code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs (4)
16-20: Well-structured refactoring to IIncrementalGenerator!The transition from
ISourceGeneratortoIIncrementalGeneratoris a good improvement that aligns with the PR's objectives. This will enhance build performance by allowing the generator to avoid reprocessing unchanged inputs.
33-33: Consider cross-platform support for the XAML file regex.Currently,
_languagesXamlRegexuses backslashes in its pattern (e.g.,@"\\Languages\\[^\\]+\.xaml$"), which may fail on Linux/macOS file paths using forward slashes.Below is a suggested diff to match both path separators:
- private readonly Regex _languagesXamlRegex = new Regex(@"\\Languages\\[^\\]+\.xaml$", RegexOptions.IgnoreCase); + private readonly Regex _languagesXamlRegex = new Regex(@"[\\/]+Languages[\\/]+[^\\/]+\.xaml$", RegexOptions.IgnoreCase);
418-421: Decide on implementation approach for unused keys.The code to filter out unused keys is commented out with a TODO, which aligns with the PR objectives to disable scanning of unused keys in release mode. However, the surrounding code (lines 345-359) collects and reports unused keys but doesn't use them to filter the generated methods.
Please confirm if this is the intended behavior or if the commented code should be implemented to filter out unused keys.
539-602: Well-structured data classes for improved code organization.The introduction of these helper classes (
MethodParameter,LocalizableStringParam,LocalizableString,PluginClassInfo) significantly improves code organization and readability. Each class has proper property definitions and constructors.
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs
Outdated
Show resolved
Hide resolved
|
I tried this version, the analyzers and the source gen do work, with one exception: it doesn't return the actual translations. Instead, the whole generated class looks like this: public static class Localize
{
/// <code>
///
/// Flow detected you have installed {0} plugins, which will require {1} to run. Would you like to download {1}?
/// {2}{2}
/// Click no if it's already installed, and you will be prompted to select the folder that contains the {1} executable
///
/// </code>
public static string runtimePluginInstalledChooseRuntimePrompt(object? arg0, object? arg1, object? arg2) => "LOCALIZATION_ERROR";
/// <code>
/// Please select the {0} executable
/// </code>
public static string runtimePluginChooseRuntimeExecutable(object? arg0) => "LOCALIZATION_ERROR";
/// <code>
/// Unable to set {0} executable path, please try from Flow's settings (scroll down to the bottom).
/// </code>
public static string runtimePluginUnableToSetExecutablePath(object? arg0) => "LOCALIZATION_ERROR";
// the rest of the class
} |
Thanks for your report. It seems that I forget to test for non-zero parameter strings. |
|
@Yusyuriv Now I have already fixed this and please check. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs (1)
33-33: 🛠️ Refactor suggestionConsider cross-platform support for the XAML file regex.
Currently,
_languagesXamlRegexuses backslashes in its pattern (e.g.,@"\\Languages\\[^\\]+\.xaml$"), which may fail on Linux/macOS file paths using forward slashes.Apply this diff to match both path separators:
- private readonly Regex _languagesXamlRegex = new Regex(@"\\Languages\\[^\\]+\.xaml$", RegexOptions.IgnoreCase); + private readonly Regex _languagesXamlRegex = new Regex(@"[\\/]+Languages[\\/]+[^\\/]+\.xaml$", RegexOptions.IgnoreCase);
🧹 Nitpick comments (2)
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs (2)
198-198: Duplicate TODO comment for usedKeys support.There's another TODO comment for used keys support. Having duplicate TODO items may lead to confusion during implementation.
Consider consolidating the TODOs into a single comprehensive comment or implementing the feature before merging.
521-533: Consider simplifying the Spacing method.The current implementation of the
Spacingmethod is complex and could be simplified. While I understand from previous comments that you prefer to keep this implementation, a simpler version would be more maintainable.A more concise implementation would be:
- private static string Spacing(int n) - { - Span<char> spaces = stackalloc char[n * 4]; - spaces.Fill(' '); - - var sb = new StringBuilder(n * 4); - foreach (var c in spaces) - { - _ = sb.Append(c); - } - - return sb.ToString(); - } + private static string Spacing(int n) + { + return new string(' ', n * 4); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs(2 hunks)
🔇 Additional comments (6)
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs (6)
55-55: Address the TODO comment for usedKeys support.The TODO comment indicates implementation for used keys is pending. This feature should be implemented, especially since the PR mentions concerns about scanning unused keys.
Are you planning to implement this feature? The commented-out code at lines 418-421 suggests this functionality exists but is disabled.
276-276: Correct implementation of null checking logic.The logic for filtering non-null classes has been properly fixed to use
&&instead of||as mentioned in a previous review comment.
16-20: Good use of XML documentation for class purpose.Adding XML documentation to the class makes the code more maintainable and helps other developers understand the generator's purpose.
41-76: Well-structured Incremental Generator implementation.The rewrite to
IIncrementalGeneratoris a good improvement as it allows for more efficient code generation. The setup with multiple providers (XAML files, invocation keys, plugin classes) that are combined is well-structured.
565-579: Good class design for the localization models.The newly added model classes (
LocalizableString,LocalizableStringParam, etc.) provide a clear structure for handling localization data, making the code more maintainable and easier to understand.
345-359:❓ Verification inconclusive
Consider the trade-offs of disabling unused key scanning in Release mode.
The code currently detects unused localization keys only in Release mode, reporting them as diagnostics. While this reduces build output in Debug mode, it may lead to unnecessary keys remaining in the codebase.
Is disabling the optimization for unused key scanning in non-Release builds intentional? According to the PR objectives, you're planning to disable scanning unused keys when optimization level is set to release, but this code seems to be doing the opposite - it only scans for unused keys in Release mode.
🏁 Script executed:
#!/bin/bash # Analyze the unused keys scanning logic across different build configurations echo "Checking when unused keys scanning is performed..." echo "Current implementation scans for unused keys only in Release mode." echo "This seems contrary to the PR objective which states 'disable the scanning of unused keys when the optimization level is set to release'."Length of output: 544
Clarify Unused Key Scanning Behavior
The current implementation performs unused localization key scanning and reports diagnostics only in Release mode. However, the PR objectives indicate that scanning should be disabled when the optimization level is set to Release. This discrepancy could lead to unwanted diagnostics during Release builds or allow unused keys to remain undetected if the intended behavior is reversed.
- Location:
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs(Lines 345–359)- Issue: The condition checks for
OptimizationLevel.Release, thereby triggering scanning in Release builds, contrary to the stated objective.- Action Requested: Please confirm whether the current behavior is intentional. If not, consider revising the conditional check to disable unused key scanning in Release mode and enable it in non-Release builds to better align with the PR objectives.
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs
Show resolved
Hide resolved
|
Thanks for your review👍 |
Important
This is initialization for
IIncrementalGeneratorwhich just ensures basic function work.IIncrementalGenerator.// TODO: Add support for usedKeysbecause both original and current codes cannot work.And for 4. I think we should not add this feature since users can use those translations anywhere by any methods?