DEMO: Localization source generators and analyzers #2723
Conversation
…ic" error, also fix it being too restrictive
|
I think if we are going to merge this pr, we will want to release this to nuget. @jjw24 @JohnTheGr8 do you know how are we going to do that similar to the plugin package? |
|
I could merge the SourceGenerators and Analyzers projects into one if it makes things easier. |
|
@Yusyuriv cool idea, this could be very helpful...
Does it not make more sense to move these two projects to a separate repository? We don't want to complicate this repository's CI work any further, and the release of the generators/analyzers should be independent of the launcher itself... (unlike the plugin project) @jjw24 thoughts? |
|
I also thought about making the class name something very short for convenience, like this, but I'm not sure if it's worth it over the readability of - Localize.newVersionTips("1.0.0")
+ L.newVersionTips("1.0.0") |
I wouldn't go with that. With modern ide Localize only requires 2-3 strokes which is not significant. And generally people don't need to use it a lot. |
|
Good idea, and all comments are valid. I confirm we should have the generators/analyzers projects independent of the launcher itself. |
WalkthroughWalkthroughThe changes introduce a set of diagnostic analyzers and code fix providers within the Flow Launcher framework, focusing on localization best practices. New project files for analyzers and source generators are created, along with various diagnostic descriptors that help identify issues related to localization in plugin development. Additionally, updates are made to existing project files to integrate these analyzers and improve localization handling throughout the application. Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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: 18
Outside diff range and nitpick comments (24)
Flow.Launcher.Analyzers/AnalyzerReleases.Unshipped.md (2)
1-6: Remove extra semicolon and approve file structureThe file structure and header provide clear context for the unshipped analyzer rules. However, there's an extra semicolon at the beginning of the file that should be removed.
Apply this diff to remove the extra semicolon:
-; Unshipped analyzer release +Unshipped analyzer releaseTools
LanguageTool
[uncategorized] ~1-~1: Loose punctuation mark.
Context: ; Unshipped analyzer release ; https://gi...(UNLIKELY_OPENING_PUNCTUATION)
Markdownlint
2-2: null
Bare URL used(MD034, no-bare-urls)
7-11: LGTM: Well-defined rules with a suggestion for clarityThe five localization rules are consistently formatted and provide clear information. The progression of rule IDs, varying severities, and descriptive notes offer a good overview of each rule's purpose and importance.
Consider adding a brief description of each severity level (Warning vs. Error) in a comment above the table. This would provide additional context for users unfamiliar with the impact of different severity levels.
Flow.Launcher.SourceGenerators/Flow.Launcher.SourceGenerators.csproj (2)
1-9: LGTM! Consider adding more project metadata.The project configuration looks good. The use of .NET Standard 2.0 aligns with the PR objectives for broader compatibility. The
EnforceExtendedAnalyzerRulesproperty is correctly set for a source generator project.Consider adding more project metadata such as
Authors,Description, andPackageTags. This will be useful if the project is published as a NuGet package in the future, as suggested in the PR comments.Example:
<PropertyGroup> <!-- existing properties --> <Authors>Flow Launcher Team</Authors> <Description>Source generators for Flow Launcher localization</Description> <PackageTags>flow-launcher;source-generator;localization</PackageTags> </PropertyGroup>
10-16: LGTM! Consider using centralized package versioning.The package references are appropriate for a source generator project, and the versions are relatively recent. The asset configuration for Microsoft.CodeAnalysis.Analyzers is correct.
For better maintainability, consider using centralized package versioning. This approach allows you to manage all package versions in a single file, making it easier to keep dependencies up-to-date across multiple projects.
Here's how you can implement it:
- Create a
Directory.Packages.propsfile in the root of your repository with the following content:<Project> <ItemGroup> <PackageVersion Include="Microsoft.CodeAnalysis.Analyzers" Version="3.3.4" /> <PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="4.9.2" /> </ItemGroup> </Project>
- Modify your .csproj file to use centralized versions:
<ItemGroup> - <PackageReference Include="Microsoft.CodeAnalysis.Analyzers" Version="3.3.4"> + <PackageReference Include="Microsoft.CodeAnalysis.Analyzers"> <PrivateAssets>all</PrivateAssets> <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> </PackageReference> - <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.9.2" /> + <PackageReference Include="Microsoft.CodeAnalysis.CSharp" /> </ItemGroup>This approach will make it easier to manage package versions across multiple projects in the future.
Flow.Launcher.Analyzers/Flow.Launcher.Analyzers.csproj (2)
1-7: LGTM! Consider dynamic version management.The project configuration looks good. The SDK, target framework, and analyzer rules are all appropriate for this type of project.
Consider implementing dynamic version management for this project. Based on previous learnings from the main Flow.Launcher project, the version number is typically updated during the CI/CD process. You might want to apply a similar approach here to ensure consistent version management across all Flow.Launcher components.
9-16: LGTM! Minor formatting suggestion.The package references are appropriate for a code analysis project, and the versions used are up-to-date.
For consistency in XML formatting, consider adding self-closing tags to the last two PackageReference elements:
- <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.9.2"/> - <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.9.2"/> + <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.9.2" /> + <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.9.2" />This change aligns with the formatting of the first PackageReference and improves overall consistency.
Flow.Launcher.SourceGenerators/AnalyzerReleases.Unshipped.md (1)
1-3: Minor formatting improvement needed in the headerThe file header looks good overall, but there's a small formatting issue:
- Remove the extra semicolon at the beginning of line 1.
- Consider adding a blank line between the header and the link for better readability.
Here's the suggested change:
-; Shipped analyzer releases -; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md +Shipped analyzer releases + +https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.mdTools
LanguageTool
[uncategorized] ~1-~1: Loose punctuation mark.
Context: ; Shipped analyzer releases ; https://git...(UNLIKELY_OPENING_PUNCTUATION)
Markdownlint
2-2: null
Bare URL used(MD034, no-bare-urls)
Flow.Launcher.Analyzers/AnalyzerDiagnostics.cs (6)
7-14: LGTM: Well-defined diagnostic for old localization API usage.The
OldLocalizationApiUseddiagnostic is appropriately defined with a clear message and suitable severity level. It effectively guides developers to use the new localization format.Consider adding a documentation comment above this field to provide more context about the old and new localization APIs. For example:
/// <summary> /// Diagnostic to identify usage of the old localization API and suggest the new `Localize` class method. /// </summary> public static readonly DiagnosticDescriptor OldLocalizationApiUsed = ...
16-23: LGTM with suggestions: Well-defined diagnostic for plugin context structure.The
ContextIsAFielddiagnostic effectively identifies when the plugin context is incorrectly defined as a field instead of a property.Consider the following improvements:
- Change the category from "Localization" to "Plugin Structure" or "Design" as it's more related to the plugin's architecture than localization.
- Add a documentation comment to provide more context about the requirement for the plugin context to be a property.
Example:
/// <summary> /// Diagnostic to ensure the plugin context is defined as a property, not a field. /// </summary> public static readonly DiagnosticDescriptor ContextIsAField = new DiagnosticDescriptor( "FLAN0002", "Plugin context is a field", "Plugin context must be at least internal static property", "Plugin Structure", DiagnosticSeverity.Error, isEnabledByDefault: true );
25-32: LGTM with suggestions: Well-defined diagnostic for static plugin context.The
ContextIsNotStaticdiagnostic effectively identifies when the plugin context is not defined as a static property.Consider the following improvements:
- Change the category from "Localization" to "Plugin Structure" or "Design" for consistency with the previous suggestion.
- Modify the message to be more specific to this diagnostic: "Plugin context must be a static property".
- Add a documentation comment to provide more context.
Example:
/// <summary> /// Diagnostic to ensure the plugin context is defined as a static property. /// </summary> public static readonly DiagnosticDescriptor ContextIsNotStatic = new DiagnosticDescriptor( "FLAN0003", "Plugin context is not static", "Plugin context must be a static property", "Plugin Structure", DiagnosticSeverity.Error, isEnabledByDefault: true );
34-41: LGTM with suggestions: Well-defined diagnostic for plugin context access modifier.The
ContextAccessIsTooRestrictivediagnostic effectively identifies when the plugin context property has an overly restrictive access modifier.Consider the following improvements:
- Change the category from "Localization" to "Plugin Structure" or "Design" for consistency.
- Modify the message to be more specific: "Plugin context property must have at least internal access".
- Add a documentation comment to provide more context.
Example:
/// <summary> /// Diagnostic to ensure the plugin context property has at least internal access. /// </summary> public static readonly DiagnosticDescriptor ContextAccessIsTooRestrictive = new DiagnosticDescriptor( "FLAN0004", "Plugin context property access modifier is too restrictive", "Plugin context property must have at least internal access", "Plugin Structure", DiagnosticSeverity.Error, isEnabledByDefault: true );
43-50: LGTM with suggestions: Well-defined diagnostic for missing plugin context declaration.The
ContextIsNotDeclareddiagnostic effectively identifies when the plugin context is not declared with the correct type and access modifiers.Consider the following improvements:
- Change the category from "Localization" to "Plugin Structure" or "Design" for consistency.
- Add a documentation comment to provide more context.
Example:
/// <summary> /// Diagnostic to ensure the plugin context is declared as an internal static property of type `PluginInitContext`. /// </summary> public static readonly DiagnosticDescriptor ContextIsNotDeclared = new DiagnosticDescriptor( "FLAN0005", "Plugin context is not declared", "Plugin context must be at least internal static property of type `PluginInitContext`", "Plugin Structure", DiagnosticSeverity.Error, isEnabledByDefault: true );
1-52: Overall: Well-structured and comprehensive set of diagnostics with minor improvement suggestions.The
AnalyzerDiagnosticsclass provides a robust set of diagnostics for Flow Launcher plugin development. The diagnostics cover critical aspects of plugin structure and localization, with appropriate severity levels and clear messages.Consider the following improvements:
- Consistently use "Plugin Structure" or "Design" as the category for structural diagnostics (FLAN0002 to FLAN0005).
- Add XML documentation comments to each diagnostic descriptor for better code documentation.
- Create a private const string for the common category to ensure consistency and ease of maintenance. For example:
public static class AnalyzerDiagnostics { private const string PluginStructureCategory = "Plugin Structure"; private const string LocalizationCategory = "Localization"; // Use these constants in the DiagnosticDescriptor constructors // ... }These changes will enhance the maintainability and clarity of the diagnostics.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj (1)
62-72: LGTM! Consider minor formatting adjustment for consistency.The addition of the Flow.Launcher.Analyzers and Flow.Launcher.SourceGenerators project references as analyzers, along with the AdditionalFiles entry for en.xaml, aligns well with the PR objectives. These changes will enable the use of the new localization source generators and analyzers in the BrowserBookmark plugin.
For consistency with the rest of the file, consider adjusting the indentation of the new ItemGroup to match the others:
- <ItemGroup> +<ItemGroup> <ProjectReference Include="..\..\Flow.Launcher.Analyzers\Flow.Launcher.Analyzers.csproj" ReferenceOutputAssembly="false" OutputItemType="Analyzer" /> <ProjectReference Include="..\..\Flow.Launcher.SourceGenerators\Flow.Launcher.SourceGenerators.csproj" ReferenceOutputAssembly="false" OutputItemType="Analyzer" /> <AdditionalFiles Include="Languages\en.xaml" /> - </ItemGroup> +</ItemGroup>Flow.Launcher.Core/Flow.Launcher.Core.csproj (1)
69-80: LGTM: Successful integration of analyzers and source generators.The changes effectively integrate the new analyzers and source generators into the project:
The new ProjectReference items for Flow.Launcher.Analyzers and Flow.Launcher.SourceGenerators are correctly configured with
ReferenceOutputAssembly="false"andOutputItemType="Analyzer". This ensures they're used as development-time tools without being included in the final build output.The AdditionalFiles item for en.xaml is appropriately added, likely to be used by the source generators for localization.
These changes align well with the PR objectives of improving localization handling and introducing analyzers for better development practices.
Consider adding a comment above the new ItemGroup to explain its purpose, e.g.:
<!-- Analyzer and source generator configuration --> <ItemGroup> ... </ItemGroup>This would improve the maintainability of the project file by clearly indicating the purpose of these additions.
Flow.Launcher.SourceGenerators/SourceGeneratorDiagnostics.cs (6)
7-14: LGTM: Well-defined descriptor with clear message.The
CouldNotFindResourceDictionariesdescriptor is well-structured and provides clear guidance.Consider adding a code fix provider for this diagnostic to automatically add the required
<AdditionalFiles />entry in the .csproj file.
16-23: Consider enhancing the diagnostic message with more context.The
CouldNotFindPluginEntryClassdescriptor is clear, but it could be more helpful.Consider expanding the message to provide more guidance, such as:
"Could not find the main class of your plugin. It must implement IPluginI18n and be public. Ensure it's in the root namespace of your plugin project."This additional information could help developers quickly identify and resolve the issue.
34-41: LGTM: Clear descriptor for non-static context property.The
ContextPropertyNotStaticdescriptor clearly identifies the issue with a non-static context property.Consider implementing a code fix provider for this diagnostic that would automatically add the
statickeyword to the property declaration.
43-50: LGTM: Clear descriptor for private context property.The
ContextPropertyIsPrivatedescriptor effectively communicates the issue with a private context property and suggests the correct accessibility levels.Consider implementing a code fix provider for this diagnostic that would offer to change the property's accessibility to either
internalorpublic.
52-59: LGTM: Consistent descriptor for protected context property.The
ContextPropertyIsProtecteddescriptor maintains consistency with the previous descriptor while addressing a different invalid accessibility level. This consistency in structure and messaging is commendable.As with the previous descriptor, consider implementing a code fix provider that offers to change the property's accessibility to either
internalorpublic.
61-68: LGTM: Useful descriptor for identifying unused localization keys.The
LocalizationKeyUnuseddescriptor is valuable for maintaining a clean codebase by identifying unused localization methods.Consider extending this functionality to provide a list of all unused localization keys in a summary diagnostic, which could be helpful for bulk cleanup operations.
Flow.Launcher.Core/Resource/Theme.cs (1)
⚠️ Incomplete Refactoring of Localization MethodsThe
Localizemethods are not implemented, and there are still multiple instances ofInternationalizationManager.Instance.GetTranslationbeing used across the codebase. This indicates that the refactoring to enhance localization handling is incomplete.Affected Files:
Flow.Launcher/CustomQueryHotkeySetting.xaml.csFlow.Launcher.SourceGenerators/Localize/LocalizeSourceGenerator.csFlow.Launcher/ViewModel/PluginViewModel.csFlow.Launcher/ViewModel/MainViewModel.csPlugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.csFlow.Launcher/PublicAPIInstance.csFlow.Launcher/App.xaml.csFlow.Launcher/MainWindow.xaml.csFlow.Launcher/CustomShortcutSetting.xaml.csFlow.Launcher/SettingPages/ViewModels/SettingsPaneProxyViewModel.csFlow.Launcher/SettingPages/ViewModels/SettingsPaneAboutViewModel.csFlow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.csFlow.Launcher/SettingPages/ViewModels/DropdownDataGeneric.csFlow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.csFlow.Launcher/SettingPages/ViewModels/SettingsPaneHotkeyViewModel.csFlow.Launcher/HotkeyControl.xaml.csFlow.Launcher/HotkeyControlDialog.xaml.csFlow.Launcher/Converters/TextConverter.csFlow.Launcher/Helper/HotKeyMapper.csFlow.Launcher.Core/Resource/TranslationConverter.csPlease ensure that all instances of
InternationalizationManager.Instance.GetTranslationare replaced with the appropriateLocalizemethod calls and that theLocalizemethods are properly implemented to maintain consistent localization handling across the application.Analysis chain
Line range hint
108-118: LGTM: Improved localization handlingThe replacement of
string.FormatandInternationalizationManager.Instance.GetTranslationwithLocalizemethod calls enhances localization handling. This change aligns with the PR objectives and likely reduces the chance of errors from mistyped keys while improving IDE support.To ensure the correctness of these changes, please run the following verification script:
This script will help verify that the
Localizemethods are correctly implemented and provide the same functionality as the previous approach usingInternationalizationManager.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of Localize methods and compare with previous approach # Test 1: Check if Localize methods are correctly implemented ast-grep --lang csharp --pattern 'public static string theme_load_failure_path_not_exists(string theme) { $$$ }' ast-grep --lang csharp --pattern 'public static string theme_load_failure_parse_error(string theme) { $$$ }' # Test 2: Compare new Localize methods with previous InternationalizationManager approach rg --type csharp 'InternationalizationManager\.Instance\.GetTranslation\("theme_load_failure_path_not_exists"\)' rg --type csharp 'InternationalizationManager\.Instance\.GetTranslation\("theme_load_failure_parse_error"\)'Length of output: 451
Script:
#!/bin/bash # Description: List all methods in the Localize class to verify implementation ast-grep --lang csharp --pattern 'public static string Localize\.(\$$_)\($_) { $$$ }' # Description: Search the entire codebase for any remaining instances of InternationalizationManager.GetTranslation rg --type csharp 'InternationalizationManager\.Instance\.GetTranslation\('Length of output: 11539
Flow.Launcher.Analyzers/Localize/OldGetTranslateAnalyzer.cs (1)
1-1: Remove unnecessary Byte Order Mark (BOM) character at the start of the file.The file appears to contain a Byte Order Mark (BOM) at the beginning, represented by the '' character before the
usingstatement on line 1. While BOM characters can be useful for encoding detection, they are generally unnecessary for UTF-8 files and may cause issues with some compilers or tools.Flow.Launcher.SourceGenerators/Localize/LocalizeSourceGenerator.cs (1)
241-245: Return after reporting diagnostics to prevent redundant code executionIn multiple places, after reporting a diagnostic, the code returns an empty string:
context.ReportDiagnostic(Diagnostic.Create(...)); return string.Empty;If the intention is to stop further execution after reporting the diagnostic, consider adding
returnstatements immediately after reporting to make the control flow clearer.Apply this diff to streamline the control flow:
context.ReportDiagnostic(Diagnostic.Create(...)); -return string.Empty; +return;Ensure the method signature and calling code handle the early return appropriately.
Also applies to: 250-254, 260-264
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (20)
- Flow.Launcher.Analyzers/AnalyzerDiagnostics.cs (1 hunks)
- Flow.Launcher.Analyzers/AnalyzerReleases.Shipped.md (1 hunks)
- Flow.Launcher.Analyzers/AnalyzerReleases.Unshipped.md (1 hunks)
- Flow.Launcher.Analyzers/Flow.Launcher.Analyzers.csproj (1 hunks)
- Flow.Launcher.Analyzers/Localize/ContextAvailabilityAnalyzer.cs (1 hunks)
- Flow.Launcher.Analyzers/Localize/ContextAvailabilityAnalyzerCodeFixProvider.cs (1 hunks)
- Flow.Launcher.Analyzers/Localize/OldGetTranslateAnalyzer.cs (1 hunks)
- Flow.Launcher.Analyzers/Localize/OldGetTranslateAnalyzerCodeFixProvider.cs (1 hunks)
- Flow.Launcher.Core/Flow.Launcher.Core.csproj (4 hunks)
- Flow.Launcher.Core/Resource/Theme.cs (5 hunks)
- Flow.Launcher.Core/Updater.cs (4 hunks)
- Flow.Launcher.SourceGenerators/AnalyzerReleases.Shipped.md (1 hunks)
- Flow.Launcher.SourceGenerators/AnalyzerReleases.Unshipped.md (1 hunks)
- Flow.Launcher.SourceGenerators/Flow.Launcher.SourceGenerators.csproj (1 hunks)
- Flow.Launcher.SourceGenerators/Localize/LocalizeSourceGenerator.cs (1 hunks)
- Flow.Launcher.SourceGenerators/SourceGeneratorDiagnostics.cs (1 hunks)
- Flow.Launcher.sln (3 hunks)
- Flow.Launcher/Languages/en.xaml (3 hunks)
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj (1 hunks)
- Plugins/Flow.Launcher.Plugin.Calculator/Flow.Launcher.Plugin.Calculator.csproj (4 hunks)
Additional context used
Learnings (2)
Flow.Launcher.Analyzers/Flow.Launcher.Analyzers.csproj (1)
Learnt from: taooceros PR: Flow-Launcher/Flow.Launcher#2616 File: Flow.Launcher/Flow.Launcher.csproj:7-7 Timestamp: 2024-06-16T22:37:00.298Z Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.Flow.Launcher.Core/Flow.Launcher.Core.csproj (1)
Learnt from: taooceros PR: Flow-Launcher/Flow.Launcher#2616 File: Flow.Launcher/Flow.Launcher.csproj:7-7 Timestamp: 2024-06-16T22:37:00.298Z Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
LanguageTool
Flow.Launcher.Analyzers/AnalyzerReleases.Shipped.md
[uncategorized] ~1-~1: Loose punctuation mark.
Context: ; Shipped analyzer releases ; https://git...(UNLIKELY_OPENING_PUNCTUATION)
Flow.Launcher.Analyzers/AnalyzerReleases.Unshipped.md
[uncategorized] ~1-~1: Loose punctuation mark.
Context: ; Unshipped analyzer release ; https://gi...(UNLIKELY_OPENING_PUNCTUATION)
Flow.Launcher.SourceGenerators/AnalyzerReleases.Shipped.md
[uncategorized] ~1-~1: Loose punctuation mark.
Context: ; Shipped analyzer releases ; https://git...(UNLIKELY_OPENING_PUNCTUATION)
Flow.Launcher.SourceGenerators/AnalyzerReleases.Unshipped.md
[uncategorized] ~1-~1: Loose punctuation mark.
Context: ; Shipped analyzer releases ; https://git...(UNLIKELY_OPENING_PUNCTUATION)
Markdownlint
Flow.Launcher.Analyzers/AnalyzerReleases.Shipped.md
2-2: null
Bare URL used(MD034, no-bare-urls)
Flow.Launcher.Analyzers/AnalyzerReleases.Unshipped.md
2-2: null
Bare URL used(MD034, no-bare-urls)
Flow.Launcher.SourceGenerators/AnalyzerReleases.Shipped.md
2-2: null
Bare URL used(MD034, no-bare-urls)
Flow.Launcher.SourceGenerators/AnalyzerReleases.Unshipped.md
2-2: null
Bare URL used(MD034, no-bare-urls)
Additional comments not posted (39)
Flow.Launcher.Analyzers/AnalyzerReleases.Unshipped.md (1)
5-6: LGTM: Table header is well-structuredThe table header clearly defines the necessary columns for documenting the analyzer rules.
Flow.Launcher.Analyzers/Flow.Launcher.Analyzers.csproj (1)
1-18: Overall, the project file looks good and is well-structured.The
Flow.Launcher.Analyzers.csprojfile is correctly set up for a .NET analyzer project. It includes appropriate SDK references, targets the right framework, and has the necessary package dependencies for code analysis.A few minor suggestions have been made:
- Consider implementing dynamic version management to align with the main Flow.Launcher project's practices.
- Minor formatting adjustments for consistency in package references.
These small improvements will enhance the project's maintainability and consistency with the broader Flow.Launcher ecosystem.
Flow.Launcher.SourceGenerators/AnalyzerReleases.Unshipped.md (2)
5-13: Table structure and content look goodThe table of new rules is well-structured and contains all the necessary information:
- Each rule has a unique ID (FLSG0001 to FLSG0007).
- All rules are categorized under "Localization" with "Warning" severity.
- The notes column provides clear references to the rule IDs.
This format makes it easy to understand the purpose and severity of each new analyzer rule.
1-13: New analyzer rules align well with PR objectivesThe introduction of these 7 new analyzer rules for localization is a significant improvement that aligns perfectly with the PR objectives. These rules will help:
- Ensure proper setup of resource dictionaries (FLSG0001)
- Verify the presence of plugin entry classes (FLSG0002)
- Enforce correct implementation of context properties (FLSG0003-FLSG0006)
- Identify unused localization keys (FLSG0007)
These rules will greatly enhance the localization process, reduce potential errors, and improve overall code quality. The rule for unused localization keys (FLSG0007) is particularly valuable for maintaining clean and efficient localization files.
Tools
LanguageTool
[uncategorized] ~1-~1: Loose punctuation mark.
Context: ; Shipped analyzer releases ; https://git...(UNLIKELY_OPENING_PUNCTUATION)
Markdownlint
2-2: null
Bare URL used(MD034, no-bare-urls)
Flow.Launcher.Analyzers/AnalyzerDiagnostics.cs (1)
1-6: LGTM: Appropriate namespace and class declaration.The namespace
Flow.Launcher.Analyzersand the public static classAnalyzerDiagnosticsare well-suited for defining analyzer diagnostics. The use ofMicrosoft.CodeAnalysisis correct for working with Roslyn analyzers.Plugins/Flow.Launcher.Plugin.Calculator/Flow.Launcher.Plugin.Calculator.csproj (3)
69-76: LGTM: Correct integration of analyzers and source generatorsThe addition of project references to
Flow.Launcher.AnalyzersandFlow.Launcher.SourceGeneratorswithReferenceOutputAssembly="false"andOutputItemType="Analyzer"is correct. This configuration ensures that these projects are used as analyzers during compilation without being directly referenced by the Calculator plugin. This aligns with the PR objectives of introducing custom analyzers and source generators for improved localization handling.
77-77: LGTM: Correct inclusion of localization file for source generationThe addition of
Languages\en.xamlas anAdditionalFilesitem is correct. This makes the English localization file available to the source generator during compilation, allowing it to generate the static localization methods as described in the PR objectives. This change is crucial for the new localization approach using source generators.
68-78: Verify consistency across other plugin projectsThe changes to integrate the new localization approach in this project look good. To ensure consistency across the codebase, it would be beneficial to verify if similar changes (adding analyzer references and the localization file as an additional file) are needed in other plugin projects.
I can help verify this. Would you like me to generate a script to check other plugin projects for consistency?
Flow.Launcher.Core/Flow.Launcher.Core.csproj (3)
16-16: LGTM: Minor formatting improvement.The addition of an empty line here improves the readability of the project file by separating different sections.
27-27: LGTM: Improved project file structure.The addition of empty lines throughout the file enhances the overall structure and readability of the project file. This change makes it easier to distinguish between different sections of the configuration.
Also applies to: 42-42, 46-46, 54-54, 63-63
Line range hint
1-81: Overall: Successful integration of analyzers and source generators with improved project structure.The changes to this project file effectively integrate the new analyzers and source generators while also improving the overall structure and readability. The modifications align well with the PR objectives of enhancing localization handling and introducing development-time tools for better coding practices.
Key points:
- New ProjectReference items for analyzers and source generators are correctly configured.
- The AdditionalFiles item for the localization file is appropriately added.
- The overall structure of the project file has been improved with better spacing between sections.
These changes should contribute to a more robust development process for the Flow Launcher project, particularly in terms of localization and code quality.
Flow.Launcher.SourceGenerators/SourceGeneratorDiagnostics.cs (3)
5-6: LGTM: Class structure is appropriate.The
SourceGeneratorDiagnosticsclass is correctly defined as public and static, which is suitable for holding constant diagnostic descriptors.Also applies to: 69-70
25-32: LGTM: Comprehensive descriptor with detailed message.The
CouldNotFindContextPropertydescriptor provides clear and detailed information about the required property, including its type and accessibility requirements. The use of a placeholder for the class name is a good practice for providing context-specific information.
1-70: Overall: Well-structured and comprehensive set of diagnostic descriptors.The
SourceGeneratorDiagnosticsclass provides a robust set of diagnostic descriptors for localization-related issues in the Flow Launcher project. The descriptors are consistently structured, clearly written, and cover a wide range of potential problems that developers might encounter.Some suggestions for further improvement:
- Consider implementing code fix providers for applicable diagnostics to streamline the resolution process.
- For the
LocalizationKeyUnuseddiagnostic, consider adding a summary diagnostic that lists all unused keys.- Ensure that these diagnostics are well-documented in the project's developer guide or README, explaining how to resolve each issue.
Great job on creating a comprehensive set of diagnostics to improve the localization process!
Flow.Launcher.Core/Updater.cs (8)
40-40: LGTM: Improved localization implementationThe change from
api.GetTranslationtoLocalizeclass methods enhances type safety and IDE support, aligning with the PR's objective of improving localization.
55-55: LGTM: Consistent localization improvementThis change follows the same pattern of using the
Localizeclass, maintaining consistency throughout the file and improving type safety.
60-60: LGTM: Continued improvement in localizationThe consistent use of
Localizeclass methods for localization strings enhances code maintainability and reduces the risk of typos in localization keys.
71-75: LGTM: Enhanced localization with dynamic contentThis change not only continues the pattern of using the
Localizeclass but also demonstrates an improvement in error message flexibility. By including parameters for the source and destination paths, the localized message can provide more specific and helpful information to the user.
82-82: LGTM: Simplified code structure with centralized localizationThe replacement of the
NewVersionTipsmethod with a direct call toLocalize.newVersionTips()simplifies the code structure while maintaining the localization functionality. This change centralizes the localization logic, making it easier to manage and update translations in the future.
86-86: LGTM: Consistent application of localization improvementsThis change continues the pattern of using the
Localizeclass for localization, maintaining consistency throughout the file and improving overall code quality.
97-99: LGTM: Improved localization and code readabilityThe changes in this segment continue the consistent use of the
Localizeclass for localization strings. Additionally, the insertion of a blank line improves code readability by clearly separating the error handling logic from the user notification.
Line range hint
1-150: Overall assessment: Successful implementation of localization improvementsThe changes in this file consistently replace
api.GetTranslationcalls with the newLocalizeclass methods, aligning perfectly with the PR's objective of improving localization. This approach enhances type safety, IDE support, and overall code maintainability. The removal of theNewVersionTipsmethod and its integration into theLocalizeclass further centralizes the localization logic.These changes will make it easier to manage and update translations in the future, reduce the risk of typos in localization keys, and improve the developer experience when working with localized strings.
Flow.Launcher.Core/Resource/Theme.cs (3)
82-86: LGTM: Improved code readabilityThe removal of unnecessary blank lines enhances code readability without affecting functionality.
89-89: LGTM: Enhanced code compactnessThe removal of the unnecessary blank line contributes to a more compact code structure without impacting functionality.
Line range hint
151-192: LGTM: Improved code style and readabilityThe changes in this segment enhance the code's readability and style:
- Removal of an unnecessary blank line improves code compactness.
- Adjusted indentation of the lambda expression in the
Array.ForEachcall provides better visual alignment and consistency.These minor adjustments contribute to a cleaner and more maintainable codebase.
Flow.Launcher.sln (3)
74-77: New projects added for source generators and analyzers.The addition of
Flow.Launcher.SourceGeneratorsandFlow.Launcher.Analyzersprojects is in line with the PR objectives. These projects will likely contain the implementation for improving localization through source generation and code analysis.
74-77: Summary of changes to Flow.Launcher.slnThe modifications to the solution file effectively integrate the new source generators and analyzers projects into the Flow Launcher solution. These changes align well with the PR objectives of improving localization through code generation and analysis. The new projects are properly configured for various build scenarios, which should facilitate development and testing across different environments.
As a next step, it would be beneficial to review the actual implementation within these new projects to ensure they fulfill the intended localization improvements.
Also applies to: 293-316
293-316: Configuration settings added for new projects.The configuration settings for
Flow.Launcher.SourceGeneratorsandFlow.Launcher.Analyzersare correctly added, covering all standard build configurations (Debug/Release) and platforms (Any CPU, x64, x86). This ensures compatibility and buildability across different environments.To ensure the new projects are correctly integrated, please verify the build process with the following script:
Flow.Launcher/Languages/en.xaml (5)
149-150: Parameter documentation added for theme loading failure.A comment has been added to document the parameter for the
theme_load_failure_path_not_existsstring resource. This improves code clarity for translators and developers.
151-152: Parameter documentation added for theme parsing error.A comment has been added to document the parameter for the
theme_load_failure_parse_errorstring resource. This enhances code readability and maintainability.
259-260: Parameter documentation added for new version notification.A comment has been added to document the parameter for the
newVersionTipsstring resource. This improves clarity for developers and translators working with version-related messages.
364-367: Parameter documentation added for portable user profile data moving failure.Comments have been added to document the parameters for the
update_flowlauncher_fail_moving_portable_user_profile_datastring resource. This enhances understanding of the placeholders used in the error message.
Line range hint
149-367: LGTM: Improved string resource documentation.The changes in this file focus on adding parameter documentation for several string resources. These additions enhance code clarity and maintainability, particularly for translators and developers working with localization. The changes do not affect the functionality of the application and are overall beneficial.
Key improvements:
- Added parameter documentation for theme loading failure messages.
- Added parameter documentation for new version notification.
- Added parameter documentation for portable user profile data moving failure message.
These changes align with best practices for maintaining localization files and will help prevent potential errors in string formatting.
Flow.Launcher.Analyzers/Localize/ContextAvailabilityAnalyzer.cs (1)
13-18: Verify the existence ofAnalyzerDiagnosticsdefinitionsEnsure that
AnalyzerDiagnostics.ContextIsAField,ContextIsNotStatic,ContextAccessIsTooRestrictive, andContextIsNotDeclaredare properly defined and accessible. Missing or inaccessible definitions will lead to compilation errors.Run the following script to confirm the definitions:
Verification successful
All required
AnalyzerDiagnosticsdefinitions are present and properly defined inAnalyzerDiagnostics.cs.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that AnalyzerDiagnostics definitions exist. # Search for AnalyzerDiagnostics definitions in the codebase. rg --type cs -A 2 'class AnalyzerDiagnostics'Length of output: 341
Script:
#!/bin/bash # Description: Verify that specific AnalyzerDiagnostics definitions exist. # Define the diagnostic descriptors to verify descriptors=( "public static readonly DiagnosticDescriptor ContextIsAField" "public static readonly DiagnosticDescriptor ContextIsNotStatic" "public static readonly DiagnosticDescriptor ContextAccessIsTooRestrictive" "public static readonly DiagnosticDescriptor ContextIsNotDeclared" ) # Iterate over each descriptor and search for its definition for descriptor in "${descriptors[@]}"; do echo "Searching for: $descriptor" rg --type cs "$descriptor" || echo "Definition not found: $descriptor" doneLength of output: 2049
Flow.Launcher.Analyzers/Localize/OldGetTranslateAnalyzerCodeFixProvider.cs (3)
16-16: LGTM!The
FixableDiagnosticIdsproperty is correctly implemented and specifies the diagnostic ID to be fixed.
18-21: LGTM!The
GetFixAllProvidermethod correctly returns theBatchFixer, enabling batch code fixes.
23-37: LGTM!The
RegisterCodeFixesAsyncmethod is well-implemented, registering the code fix action appropriately.Flow.Launcher.Analyzers/Localize/OldGetTranslateAnalyzer.cs (1)
52-52: Syntax error: Replaceandwith&&for logical AND operation.In C#, the logical AND operation uses the
&&operator, notand. Usingandwill result in a syntax error. Please replaceandwith&&in the conditional statement on line 52.Apply this diff to fix the syntax error:
- else if (IsTranslateCall(methodSymbol) and GetFirstArgumentStringValue(invocationExpr) is string translationKey) + else if (IsTranslateCall(methodSymbol) && GetFirstArgumentStringValue(invocationExpr) is string translationKey)Likely invalid or redundant comment.
Flow.Launcher.SourceGenerators/Localize/LocalizeSourceGenerator.cs (1)
339-407: Validate format strings to prevent runtime errorsIn the
GenerateMethodfunction, format strings are generated based on the presence of placeholders like{0},{1}, etc. If the number of placeholders does not match the provided arguments, it can lead to runtime exceptions.Run the following script to check if all format strings have matching placeholders and arguments:
This script will help identify any discrepancies between placeholders and arguments in the generated methods.
| context.ReportDiagnostic(Diagnostic.Create( | ||
| AnalyzerDiagnostics.ContextIsNotStatic, | ||
| contextProperty.DeclaringSyntaxReferences[0].GetSyntax().GetLocation() | ||
| )); |
There was a problem hiding this comment.
Prevent potential IndexOutOfRangeException when accessing DeclaringSyntaxReferences
Accessing DeclaringSyntaxReferences[0] without checking if the array has elements can lead to a runtime exception if the array is empty. Ensure that the array has elements before accessing the first item.
Apply this diff to add a null check:
if (!contextProperty.IsStatic)
{
- context.ReportDiagnostic(Diagnostic.Create(
- AnalyzerDiagnostics.ContextIsNotStatic,
- contextProperty.DeclaringSyntaxReferences[0].GetSyntax().GetLocation()
- ));
+ var syntaxReference = contextProperty.DeclaringSyntaxReferences.FirstOrDefault();
+ if (syntaxReference != null)
+ {
+ context.ReportDiagnostic(Diagnostic.Create(
+ AnalyzerDiagnostics.ContextIsNotStatic,
+ syntaxReference.GetSyntax().GetLocation()
+ ));
+ }
}Repeat similar checks for other instances where DeclaringSyntaxReferences[0] is accessed.
Also applies to: 57-59, 85-88
| var contextProperty = classDeclaration.Members.OfType<PropertyDeclarationSyntax>() | ||
| .Select(p => semanticModel.GetDeclaredSymbol(p)) | ||
| .FirstOrDefault(p => p?.Type.Name is PluginContextTypeName); |
There was a problem hiding this comment.
Fix incorrect string comparison using 'is' operator
The code uses the is operator for string comparison, which is incorrect. The is operator checks for type compatibility, not value equality. Use == for string comparison to ensure correct functionality.
Apply this diff to fix the comparison:
- .FirstOrDefault(p => p?.Type.Name is PluginContextTypeName);
+ .FirstOrDefault(p => p?.Type.Name == PluginContextTypeName);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.
| var contextProperty = classDeclaration.Members.OfType<PropertyDeclarationSyntax>() | |
| .Select(p => semanticModel.GetDeclaredSymbol(p)) | |
| .FirstOrDefault(p => p?.Type.Name is PluginContextTypeName); | |
| var contextProperty = classDeclaration.Members.OfType<PropertyDeclarationSyntax>() | |
| .Select(p => semanticModel.GetDeclaredSymbol(p)) | |
| .FirstOrDefault(p => p?.Type.Name == PluginContextTypeName); |
| .OfType<FieldDeclarationSyntax>() | ||
| .SelectMany(f => f.Declaration.Variables) | ||
| .Select(f => semanticModel.GetDeclaredSymbol(f)) | ||
| .FirstOrDefault(f => f is IFieldSymbol fs && fs.Type.Name is PluginContextTypeName); |
There was a problem hiding this comment.
Fix incorrect string comparison using 'is' operator
Similar to the previous issue, the is operator is incorrectly used for comparing strings. Replace is with == to compare string values correctly.
Apply this diff to fix the comparison:
- .FirstOrDefault(f => f is IFieldSymbol fs && fs.Type.Name is PluginContextTypeName);
+ .FirstOrDefault(f => f is IFieldSymbol fs && fs.Type.Name == PluginContextTypeName);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.
| .FirstOrDefault(f => f is IFieldSymbol fs && fs.Type.Name is PluginContextTypeName); | |
| .FirstOrDefault(f => f is IFieldSymbol fs && fs.Type.Name == PluginContextTypeName); |
|
|
||
| public override void Initialize(AnalysisContext context) | ||
| { | ||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); |
There was a problem hiding this comment.
Exclude generated code from analysis
Currently, the analyzer is configured to analyze generated code, which may produce unnecessary diagnostics and impact performance. Typically, analyzers exclude generated code to focus on user-written code.
Update the configuration to exclude generated code:
context.ConfigureGeneratedCodeAnalysis(
- GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics
+ GeneratedCodeAnalysisFlags.None
);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.
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); | |
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); |
| private static bool IsPluginEntryClass(INamedTypeSymbol namedTypeSymbol) => | ||
| namedTypeSymbol?.Interfaces.Any(i => i.Name == PluginInterfaceName) ?? false; |
There was a problem hiding this comment.
Compare interface symbols using SymbolEqualityComparer
When checking if a class implements an interface, comparing symbols directly is more reliable than comparing names. This approach accounts for namespace differences and ensures accurate type comparisons.
Refactor the IsPluginEntryClass method:
-private static bool IsPluginEntryClass(INamedTypeSymbol namedTypeSymbol) =>
- namedTypeSymbol?.Interfaces.Any(i => i.Name == PluginInterfaceName) ?? false;
+private static bool IsPluginEntryClass(INamedTypeSymbol namedTypeSymbol)
+{
+ var pluginInterface = context.Compilation.GetTypeByMetadataName("YourNamespace.IPluginI18n");
+ return namedTypeSymbol.Interfaces.Contains(pluginInterface, SymbolEqualityComparer.Default);
+}Replace "YourNamespace.IPluginI18n" with the fully qualified name of the IPluginI18n interface.
Committable suggestion was skipped due to low confidence.
| public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) | ||
| { | ||
| var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); | ||
|
|
||
| var diagnostic = context.Diagnostics.First(); | ||
| var diagnosticSpan = diagnostic.Location.SourceSpan; | ||
|
|
||
| if (diagnostic.Id == AnalyzerDiagnostics.ContextIsAField.Id) | ||
| { | ||
| context.RegisterCodeFix( | ||
| CodeAction.Create( | ||
| title: "Replace with static property", | ||
| createChangedDocument: _ => Task.FromResult(FixContextIsAFieldError(context, root, diagnosticSpan)), | ||
| equivalenceKey: AnalyzerDiagnostics.ContextIsAField.Id | ||
| ), | ||
| diagnostic | ||
| ); | ||
| } | ||
| else if (diagnostic.Id == AnalyzerDiagnostics.ContextIsNotStatic.Id) | ||
| { | ||
| context.RegisterCodeFix( | ||
| CodeAction.Create( | ||
| title: "Make static", | ||
| createChangedDocument: _ => Task.FromResult(FixContextIsNotStaticError(context, root, diagnosticSpan)), | ||
| equivalenceKey: AnalyzerDiagnostics.ContextIsNotStatic.Id | ||
| ), | ||
| diagnostic | ||
| ); | ||
| } | ||
| else if (diagnostic.Id == AnalyzerDiagnostics.ContextAccessIsTooRestrictive.Id) | ||
| { | ||
| context.RegisterCodeFix( | ||
| CodeAction.Create( | ||
| title: "Make internal", | ||
| createChangedDocument: _ => Task.FromResult(FixContextIsTooRestricted(context, root, diagnosticSpan)), | ||
| equivalenceKey: AnalyzerDiagnostics.ContextAccessIsTooRestrictive.Id | ||
| ), | ||
| diagnostic | ||
| ); | ||
| } | ||
| else if (diagnostic.Id == AnalyzerDiagnostics.ContextIsNotDeclared.Id) | ||
| { | ||
| context.RegisterCodeFix( | ||
| CodeAction.Create( | ||
| title: "Declare context property", | ||
| createChangedDocument: _ => Task.FromResult(FixContextNotDeclared(context, root, diagnosticSpan)), | ||
| equivalenceKey: AnalyzerDiagnostics.ContextIsNotDeclared.Id | ||
| ), | ||
| diagnostic | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Process all diagnostics in RegisterCodeFixesAsync
Currently, the method RegisterCodeFixesAsync processes only the first diagnostic from context.Diagnostics. To ensure that all diagnostics are addressed, consider iterating over all diagnostics.
Apply this refactor to process each diagnostic:
public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
- var diagnostic = context.Diagnostics.First();
- var diagnosticSpan = diagnostic.Location.SourceSpan;
-
- if (diagnostic.Id == AnalyzerDiagnostics.ContextIsAField.Id)
- {
- // Register code fix for this diagnostic
- }
- // Other diagnostic handling...
+ foreach (var diagnostic in context.Diagnostics)
+ {
+ var diagnosticSpan = diagnostic.Location.SourceSpan;
+
+ if (diagnostic.Id == AnalyzerDiagnostics.ContextIsAField.Id)
+ {
+ // Register code fix for this diagnostic
+ }
+ // Other diagnostic handling...
+ }
}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.
| public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) | |
| { | |
| var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); | |
| var diagnostic = context.Diagnostics.First(); | |
| var diagnosticSpan = diagnostic.Location.SourceSpan; | |
| if (diagnostic.Id == AnalyzerDiagnostics.ContextIsAField.Id) | |
| { | |
| context.RegisterCodeFix( | |
| CodeAction.Create( | |
| title: "Replace with static property", | |
| createChangedDocument: _ => Task.FromResult(FixContextIsAFieldError(context, root, diagnosticSpan)), | |
| equivalenceKey: AnalyzerDiagnostics.ContextIsAField.Id | |
| ), | |
| diagnostic | |
| ); | |
| } | |
| else if (diagnostic.Id == AnalyzerDiagnostics.ContextIsNotStatic.Id) | |
| { | |
| context.RegisterCodeFix( | |
| CodeAction.Create( | |
| title: "Make static", | |
| createChangedDocument: _ => Task.FromResult(FixContextIsNotStaticError(context, root, diagnosticSpan)), | |
| equivalenceKey: AnalyzerDiagnostics.ContextIsNotStatic.Id | |
| ), | |
| diagnostic | |
| ); | |
| } | |
| else if (diagnostic.Id == AnalyzerDiagnostics.ContextAccessIsTooRestrictive.Id) | |
| { | |
| context.RegisterCodeFix( | |
| CodeAction.Create( | |
| title: "Make internal", | |
| createChangedDocument: _ => Task.FromResult(FixContextIsTooRestricted(context, root, diagnosticSpan)), | |
| equivalenceKey: AnalyzerDiagnostics.ContextAccessIsTooRestrictive.Id | |
| ), | |
| diagnostic | |
| ); | |
| } | |
| else if (diagnostic.Id == AnalyzerDiagnostics.ContextIsNotDeclared.Id) | |
| { | |
| context.RegisterCodeFix( | |
| CodeAction.Create( | |
| title: "Declare context property", | |
| createChangedDocument: _ => Task.FromResult(FixContextNotDeclared(context, root, diagnosticSpan)), | |
| equivalenceKey: AnalyzerDiagnostics.ContextIsNotDeclared.Id | |
| ), | |
| diagnostic | |
| ); | |
| } | |
| } | |
| public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) | |
| { | |
| var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); | |
| foreach (var diagnostic in context.Diagnostics) | |
| { | |
| var diagnosticSpan = diagnostic.Location.SourceSpan; | |
| if (diagnostic.Id == AnalyzerDiagnostics.ContextIsAField.Id) | |
| { | |
| context.RegisterCodeFix( | |
| CodeAction.Create( | |
| title: "Replace with static property", | |
| createChangedDocument: _ => Task.FromResult(FixContextIsAFieldError(context, root, diagnosticSpan)), | |
| equivalenceKey: AnalyzerDiagnostics.ContextIsAField.Id | |
| ), | |
| diagnostic | |
| ); | |
| } | |
| else if (diagnostic.Id == AnalyzerDiagnostics.ContextIsNotStatic.Id) | |
| { | |
| context.RegisterCodeFix( | |
| CodeAction.Create( | |
| title: "Make static", | |
| createChangedDocument: _ => Task.FromResult(FixContextIsNotStaticError(context, root, diagnosticSpan)), | |
| equivalenceKey: AnalyzerDiagnostics.ContextIsNotStatic.Id | |
| ), | |
| diagnostic | |
| ); | |
| } | |
| else if (diagnostic.Id == AnalyzerDiagnostics.ContextAccessIsTooRestrictive.Id) | |
| { | |
| context.RegisterCodeFix( | |
| CodeAction.Create( | |
| title: "Make internal", | |
| createChangedDocument: _ => Task.FromResult(FixContextIsTooRestricted(context, root, diagnosticSpan)), | |
| equivalenceKey: AnalyzerDiagnostics.ContextAccessIsTooRestrictive.Id | |
| ), | |
| diagnostic | |
| ); | |
| } | |
| else if (diagnostic.Id == AnalyzerDiagnostics.ContextIsNotDeclared.Id) | |
| { | |
| context.RegisterCodeFix( | |
| CodeAction.Create( | |
| title: "Declare context property", | |
| createChangedDocument: _ => Task.FromResult(FixContextNotDeclared(context, root, diagnosticSpan)), | |
| equivalenceKey: AnalyzerDiagnostics.ContextIsNotDeclared.Id | |
| ), | |
| diagnostic | |
| ); | |
| } | |
| } | |
| } |
| { | ||
| var split = v.Expression.ToString().Split('.'); | ||
| if (split.Length < 2) return; | ||
| if (!(split[0] is ClassName)) return; |
There was a problem hiding this comment.
Fix incorrect string comparison operator
At line 57, the code uses the is operator to compare strings:
if (!(split[0] is ClassName)) return;The is operator checks for type compatibility, which is not suitable for string comparison. You should use != to compare strings for inequality.
Apply this diff to correct the string comparison:
-if (!(split[0] is ClassName)) return;
+if (split[0] != ClassName) return;| catch | ||
| { | ||
| // ignore | ||
| } |
There was a problem hiding this comment.
Avoid silently catching exceptions
In the catch block starting at line 183, exceptions are caught and ignored without any handling:
catch
{
// ignore
}Silently swallowing exceptions can make debugging difficult and obscure underlying issues. Consider logging the exception or providing diagnostic information to aid in troubleshooting.
Apply this diff to log the exception:
catch (Exception ex)
{
- // ignore
+ Console.WriteLine($"Exception in ProcessXamlFileComment: {ex.Message}");
}Alternatively, use the context.ReportDiagnostic method to report the exception as a diagnostic.
| var index = int.Parse(param.Attribute("index")?.Value ?? "-1"); | ||
| var paramName = param.Attribute("name")?.Value; | ||
| var paramType = param.Attribute("type")?.Value; | ||
| if (index < 0 || paramName is null || paramType is null) continue; |
There was a problem hiding this comment.
Safely parse integer values to prevent exceptions
At line 175, the code uses int.Parse to convert a string to an integer:
var index = int.Parse(param.Attribute("index")?.Value ?? "-1");If the index attribute contains a non-integer value, this will throw a FormatException. Use int.TryParse to safely parse the integer and handle any parsing errors gracefully.
Apply this diff to safely parse the index:
-var index = int.Parse(param.Attribute("index")?.Value ?? "-1");
+int index;
+if (!int.TryParse(param.Attribute("index")?.Value, out index))
+{
+ index = -1; // or handle the error as appropriate
+}| { | ||
| return classDeclaration.BaseList?.Types.Any(v => interfaceName == v.ToString()) is true; | ||
| } |
There was a problem hiding this comment.
Use semantic model for interface implementation checks
In the method DoesClassImplementInterface, the code checks if a class implements an interface using string comparison:
return classDeclaration.BaseList?.Types.Any(v => interfaceName == v.ToString()) is true;String comparisons can be unreliable due to formatting variations. Instead, use the semantic model to accurately determine if the class implements the interface.
Consider refactoring the method to use the semantic model:
var semanticModel = context.Compilation.GetSemanticModel(classDeclaration.SyntaxTree);
var classSymbol = semanticModel.GetDeclaredSymbol(classDeclaration);
return classSymbol.Interfaces.Any(i => i.Name == interfaceName);
Warning
This is just a demonstration to gather feedback. This isn't necessarily going to make it into the codebase. This PR shouldn't be merged, at least in the current state.
The problem
Currently, localization is located in .xaml files and used in code like this:
Or like this, if the string has variables inside:
This has a few issues:
en.xaml, look it up there, and copy the key from there. It's very inconvenient.Proposed solution
This PR implements a source generator that takes
en.xamlfile, parses it and generates a static class with methods. Each method's name is a key fromen.xaml, and the return value is the translated string. Optionally, the method receives arguments if the translated value has placeholders ({0}). It also generates doc comments so you could hover a method and see what the actual string looks like. Here's an example of generated code:This is used like this:
The Analyzers project includes several diagnostics and automatic fixes for them needed for the source generator to work. For example, when used in plugin projects,
Localizeclass expects the main class of your plugin (the one implementingIPluginI18n) to have aninternalor apublicstatic property of typePluginInitContext. It will use that property in the generated code like this:The analyzers will look at the main class of the plugin and show errors when it's impossible to generate the
Localizeclass. A few examples:This property is
privateand isn'tstatic, so it's unavailable from theLocalizeclass:This property is
static, but it's stillprivate, so it's unavailable from theLocalizeclass:This is a private field, so it's unavailable from the
Localizeclass:There's also an analyzer that warns about using the old localization method:
All of these offer an automatic code fix (Alt+Enter in Rider).
To add the source generator and the analyzers to a project, this code should be added to .csproj file:
<AdditionalFiles />is needed because source generators don't have access to files unless they're specified in<AdditionalFiles />.Unfortunately, I haven't managed to make analyzers work in Visual Studio, but they work well in Rider and VS Code.Apparently, Visual Studio allows using only .NET Standard 2.0 for analyzers. Modified the code for .NET Standard 2.0, it now works in Visual Studio.What's included in this PR
Flow.Launcher.Coreproject uses the source-generatedLocalizeclass everywhere it was usingInternationalizationManager.Instance.GetTranslation()plugins/Flow.Launcher.Plugin.BrowserBookmarkandplugins/Flow.Launcher.Plugin.Calculatorhave the source generator and the analyzers included, but no other changes introduced, so you could see for yourself how it works if you're interested to try. They're intentionally left in a broken state to show that the analyzer prevents them from building until the errors preventing the source generator from generating theLocalizeclass are fixed.What now
I don't know, I guess I just wanted to hear what you think about all this. It has been a great learning exercise for me either way.