Add FLLUseDependencyInjection property group & Move all constants to Shared project #18
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new shared project for localization, adding centralized constants and helper methods while updating analyzer and source generator projects to reference this shared project. The changes refactor hardcoded strings to use a new Changes
Sequence Diagram(s)sequenceDiagram
participant Analyzer
participant Config as AnalyzerConfigOptionsProvider
Analyzer->>Config: Retrieve "FLLUseDependencyInjection" value
alt DI Enabled
Analyzer-->>Analyzer: Exit early (skip further context analysis)
else DI Disabled
Analyzer->>Analyzer: Continue checking context properties using Constants
end
sequenceDiagram
participant Generator
participant Config as AnalyzerConfigOptionsProvider
Generator->>Config: Retrieve "FLLUseDependencyInjection" value
alt DI Enabled
Generator-->>Generator: Use DI configuration to generate localization methods
else DI Disabled
Generator->>Generator: Retrieve pluginInfo and generate methods with Constants
end
Possibly related PRs
Poem
🪧 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 (2)
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs (2)
79-83: Nested tuple complexity
Deeply nested tuples can harm readability. A small record or class might be more maintainable for passing multiple elements.
693-693: Negated condition readability
Consider simplifyingif (!(string.IsNullOrEmpty(getTranslation)))to the more typical form below.-if (!(string.IsNullOrEmpty(getTranslation))) +if (!string.IsNullOrEmpty(getTranslation))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Flow.Launcher.Localization.Analyzers/Flow.Launcher.Localization.Analyzers.csproj(1 hunks)Flow.Launcher.Localization.Analyzers/Localize/ContextAvailabilityAnalyzer.cs(5 hunks)Flow.Launcher.Localization.Analyzers/Localize/OldGetTranslateAnalyzerCodeFixProvider.cs(0 hunks)Flow.Launcher.Localization.Shared/Constants.cs(1 hunks)Flow.Launcher.Localization.Shared/Flow.Launcher.Localization.Shared.csproj(1 hunks)Flow.Launcher.Localization.Shared/Helper.cs(1 hunks)Flow.Launcher.Localization.SourceGenerators/Flow.Launcher.Localization.SourceGenerators.csproj(1 hunks)Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs(14 hunks)Flow.Launcher.Localization.slnx(1 hunks)Flow.Launcher.Localization/Flow.Launcher.Localization.csproj(1 hunks)
💤 Files with no reviewable changes (1)
- Flow.Launcher.Localization.Analyzers/Localize/OldGetTranslateAnalyzerCodeFixProvider.cs
🔇 Additional comments (27)
Flow.Launcher.Localization.slnx (1)
3-3: Solution structure correctly updated to include new Shared project.The addition of the Shared project to the solution file aligns with the PR objectives to centralize constants and facilitate dependency injection configuration.
Flow.Launcher.Localization.SourceGenerators/Flow.Launcher.Localization.SourceGenerators.csproj (1)
18-20: Project reference to Shared project correctly added.The Source Generators project now properly references the Shared project, allowing access to centralized constants and the dependency injection configuration helper.
Flow.Launcher.Localization.Analyzers/Flow.Launcher.Localization.Analyzers.csproj (1)
19-21: Project reference to Shared project correctly added.The Analyzers project now properly references the Shared project, allowing access to centralized constants and the dependency injection configuration helper.
Flow.Launcher.Localization.Shared/Helper.cs (1)
1-17: Helper extension method implemented correctly.The implementation of
GetFLLUseDependencyInjectionextension method forAnalyzerConfigOptionsProvidercorrectly retrieves the configuration property and defaults tofalsewhen not specified. This facilitates the transition fromInternationalizationManager.Instanceto dependency injection as outlined in the PR objectives.Flow.Launcher.Localization.Shared/Flow.Launcher.Localization.Shared.csproj (1)
1-14: Well-structured shared project configurationThe new shared project is properly configured with appropriate settings for a .NET Standard 2.0 library that will be referenced by analyzer projects. The use of
EnforceExtendedAnalyzerRulesproperty is good practice for Roslyn-related components.Flow.Launcher.Localization/Flow.Launcher.Localization.csproj (2)
19-21: Correctly added reference to the new shared projectThe project reference to the new Shared project has been added with appropriate
PrivateAssetsconfiguration, which is consistent with the other project references.
33-37: Properly including the shared DLL in the packageThe shared DLL is correctly added to the package with the same configuration as the other DLLs, ensuring it will be available to consumers of the package.
Flow.Launcher.Localization.Shared/Constants.cs (1)
1-18: Good centralization of constantsMoving these constants to a shared project improves maintainability and reduces duplication across the codebase. The organization of constants is clean and well-structured.
One note: the regex for
LanguagesXamlRegexis using a compiled pattern which is generally good for performance since this regex will likely be used repeatedly.Flow.Launcher.Localization.Analyzers/Localize/ContextAvailabilityAnalyzer.cs (5)
3-3: Appropriate usage of the shared namespaceThe addition of the
usingstatement for the shared namespace allows clean access to the centralized constants.
30-34: Well-implemented dependency injection checkThis code correctly implements the PR objective to add dependency injection support. The early return pattern is efficient and prevents unnecessary processing when dependency injection is enabled.
44-44: Replaced hardcoded string with constant referenceGood refactoring to use the constant from the shared project instead of a hardcoded string.
73-73: Replaced hardcoded string with constant referenceGood refactoring to use the constant from the shared project instead of a hardcoded string.
95-95: Replaced hardcoded string with constant referenceGood refactoring to use the constant from the shared project instead of a hardcoded string.
Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs (14)
8-8: Centralized dependencies
Referencing the shared localization library is a good step towards removing magic strings in this generator.
12-12: New reference for analyzer diagnostics
Addingusing Microsoft.CodeAnalysis.Diagnostics;appears valid and aligns with incremental generator requirements.
41-41: Use of a centralized regex
Replacing the inline pattern withConstants.LanguagesXamlRegeximproves maintainability and consistency.
66-68: Loading analyzer config
IntroducingAnalyzerConfigOptionsProviderinto the pipeline is a fine approach. Consider how the code behaves if the config is missing or returns unexpected values.Would you like a script to check code references for fallback logic to handle missing config?
104-105: Dependency injection flag
RetrievingFLLUseDependencyInjectionfrom AnalyzerConfig ensures the default value is used correctly. Please verify the fallback scenario remains as intended.
106-111: Conditional plugin info retrieval
Bypassing plugin info when using DI fits your design goals, avoiding unnecessary property checks.
154-158: Centralized URIs
SubstitutingConstants.SystemPrefixUriandConstants.XamlPrefixUriremoves hardcoded strings for clarity and consistency.
178-185: UsingConstants.XamlTagandConstants.KeyTag
Referring to these constants avoids string typos and promotes consistent usage across the project.
423-423: Class name check
Checking whetherparts[0]equalsConstants.ClassNamehelps ensure the generator only processes valid calls.
439-439: Plugin interface name
UtilizingConstants.PluginInterfaceNameinstead of a raw string is safer and more coherent.
447-447: Context type name
The move toConstants.PluginContextTypeNamehelps avoid mistakes and aligns with the broader constants-based strategy.
600-613: StaticApiproperty with DI
AcquiringIPublicAPIvia dependency injection is neatly implemented. If multiple threads access this static property in parallel, consider verifying thread safety.Would you like a script to check for parallel usage references of this property?
624-625: Doc comments and translation method
These lines neatly tie in the new translation approach while generating doc comments.
630-630: Constant-based output file name
Using${Constants.ClassName}.{assemblyName}.g.csfor your generated source is an excellent way to unify naming.
There was a problem hiding this comment.
I left a few comments and also added a file. With that file, there's no need for the users of this package to manually specify
<CompilerVisibleProperty Include="FLLUseDependencyInjection" />in their projects, it will be available by default. Please check out the comments and verify that on your end it also now works without manually specifying CompilerVisibleProperty.
| var configOptions = data.Item1.Item1.ConfigOptionsProvider; | ||
| var pluginClasses = data.Item1.Item1.Item1.PluginClassInfos; | ||
| var usedKeys = data.Item1.Item1.Item1.Item1.InvocationKeys; | ||
| var localizedStrings = data.Item1.Item1.Item1.Item1.LocalizableStrings; |
There was a problem hiding this comment.
I think those Item1 are getting out of hand. Isn't there a way to access them by their actual names?
There was a problem hiding this comment.
I really do not know how to name those things, like ((((ImmutableArray<LocalizableString> LocalizableStrings, ImmutableHashSet<string> InvocationKeys), ImmutableArray<PluginClassInfo> PluginClassInfos), AnalyzerConfigOptionsProvider ConfigOptionsProvider), Compilation Compilation) such type.
Use Others or Left? I have no idea. If you have any good name for me, please let me know.
There was a problem hiding this comment.
I was just curious if it's possible to avoid using tuples altogether and just use a record:
private void Execute(SourceProductionContext context, GeneratorInputs data)
{I don't have in-depth knowledge about source generators, so I don't know if it's possible or not. Let's leave it as is for now, and just remember that this is something that should probably change in the future.
There was a problem hiding this comment.
It seems that .NetCore2.0 cannot support record....
It works well on my device. |
Do you think we also need to add Developers do not need to add this file manually since this is the default place for xaml language file. |
Are we assuming that English is always going to be the default language? I think it's a pretty safe assumption to have, so yeah, we should add it to the .props file. |
IIRC, FL sets English as the default language. So all plugins should provide English strings. |
|
@Yusyuriv Or, one more thing. Because I config 
I have tried to add I have also checked this configuration: And it cannot work either. |
|
I don't think it being added as an analyzer is a problem. It doesn't actually contain any analyzers, so it doesn't do anything, and this project will be installed as a dependency only during development of plugins, it won't get published alongside the plugin. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Flow.Launcher.Localization/build/Flow.Launcher.Localization.props (3)
3-3: Consider adding a comment explaining the property's purpose.While the implementation is correct, adding a comment explaining that
FLLUseDependencyInjectioncontrols whether to use dependency injection or the static instance would improve maintainability and help future developers understand the purpose of this property.- <CompilerVisibleProperty Include="FLLUseDependencyInjection" /> + <!-- Controls whether to use Ioc.Default.GetRequiredService (true) or InternationalizationManager.Instance (false) --> + <CompilerVisibleProperty Include="FLLUseDependencyInjection" />
4-4: Consider documenting the expected file path structure.The hardcoded path
Languages\en.xamlassumes a specific project structure. Consider adding documentation or a comment to clarify this assumption for users of the package.- <AdditionalFiles Include="Languages\en.xaml" /> + <!-- Default English language file - place your localization files in a "Languages" folder at your project root --> + <AdditionalFiles Include="Languages\en.xaml" />
2-5: Consider adding a PropertyGroup for the default property value.The PR objectives mention that
FLLUseDependencyInjectionhas a default value offalse, but this isn't explicitly set in the file. Consider adding a PropertyGroup to define this default value.<Project> + <PropertyGroup> + <FLLUseDependencyInjection>false</FLLUseDependencyInjection> + </PropertyGroup> <ItemGroup> <CompilerVisibleProperty Include="FLLUseDependencyInjection" /> <AdditionalFiles Include="Languages\en.xaml" /> </ItemGroup> </Project>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Localization/build/Flow.Launcher.Localization.props(1 hunks)
🔇 Additional comments (1)
Flow.Launcher.Localization/build/Flow.Launcher.Localization.props (1)
1-6: Good implementation of automatic property configuration.This props file effectively adds the
FLLUseDependencyInjectioncompiler visible property and sets up the default English language file, which aligns well with the PR objectives to simplify adoption of dependency injection and standardize localization across projects.
|
Thanks for your quick review👍 |
Add FLLUseDependencyInjection property group
Since
InternationalizationManager.Instanceis on the way to be deprecated (Flow-Launcher/Flow.Launcher#3276), we should move toIoc.Default.GetRequiredService.To let this can be used for not only core project, we should use property
FLLUseDependencyInjectioninstead of core assembly check. The default value isfalse.Move all constants to Shared project
Improve code quality.
Test
When we add these in the project
Source generator will use dependency injection to get translation
And you do not need to have
public static Contextin your main class. (Tested with no build issue & No warning)