Avoid using magic strings

[alexeyzimarev]

No description provided.

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Replace magic strings with symbol-based type comparisons in analyzers

✨ Enhancement

Walkthroughs

Description

• Replace string-based type comparisons with symbol-based comparisons
• Add KnownTypeSymbols cache for efficient symbol resolution per compilation
• Implement refactoring-safe analyzer with fallback string comparison
• Improve code generator robustness with symbol-based attribute detection

Diagram

flowchart LR
  A["String-based<br/>Type Matching"] -->|"Refactor to"| B["Symbol-based<br/>Comparison"]
  B -->|"with Fallback"| C["String Comparison<br/>if Symbol Unavailable"]
  D["KnownTypeSymbols<br/>Cache"] -->|"Resolves Once<br/>Per Compilation"| B
  E["EventUsageAnalyzer"] -->|"Uses"| D
  F["TypeMappingsGenerator"] -->|"Uses"| B
  G["ConsumeContextConverterGenerator"] -->|"Uses"| B

Loading

File Changes

1. src/Core/gen/Eventuous.Shared.Generators/Constants.cs 📝 Documentation +14/-2

Document constants and symbol comparison strategy

• Added comprehensive XML documentation explaining constant usage and symbol-based comparison
 preference
• Clarified that constants are used for fallback string comparison when symbol resolution
 unavailable
• Improved formatting and spacing for better readability

src/Core/gen/Eventuous.Shared.Generators/Constants.cs

---

2. src/Core/gen/Eventuous.Shared.Generators/EventUsageAnalyzer.cs ✨ Enhancement +109/-34

Implement symbol-based type comparison with caching

• Introduced KnownTypeSymbols sealed class to cache well-known type symbols resolved once per
 compilation
• Modified Initialize to use RegisterCompilationStartAction for efficient symbol resolution
• Updated all analyzer methods to accept KnownTypeSymbols parameter and use symbol comparison with
 string fallback
• Refactored IsAggregate, IsState, IsFunctionalServiceAct, and HasEventTypeAttribute methods
 to prefer symbol comparison
• Enhanced GetExplicitRegistrations to use symbol comparison for TypeMapper detection

src/Core/gen/Eventuous.Shared.Generators/EventUsageAnalyzer.cs

---

3. src/Core/gen/Eventuous.Shared.Generators/TypeMappingsGenerator.cs ✨ Enhancement +70/-15

Add symbol-based attribute detection to generator

• Added symbol resolution for EventTypeAttribute at compilation start
• Introduced TransformInput struct to carry both context and resolved symbol
• Created TransformWithSymbol method to perform attribute detection using symbol comparison
• Updated GetEventTypeAttribute and TryGetEventTypeName to accept symbol parameter and prefer
 symbol comparison
• Modified DiscoverFromCompilation to accept and use resolved symbol for attribute detection

src/Core/gen/Eventuous.Shared.Generators/TypeMappingsGenerator.cs

---

View more (1)

4. src/Core/gen/Eventuous.Subscriptions.Generators/ConsumeContextConverterGenerator.cs ✨ Enhancement +43/-17

Implement symbol-based interface detection in generator

• Added InterfaceFqn constant for fully qualified interface name
• Resolved IMessageConsumeContext<> symbol at compilation start
• Refactored Transform to return context for deferred processing
• Created TransformWithSymbol method to perform interface detection using symbol comparison
• Updated IsTargetInterface and TryExtractTypeArgFromIMessageConsumeContext to use symbol
 comparison with fallback

src/Core/gen/Eventuous.Subscriptions.Generators/ConsumeContextConverterGenerator.cs

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. m and ct names 📘 Rule violation ✓ Correctness

Description

New/modified code introduces non-iterator single-letter locals like m and ct, which reduces
self-documentation and readability. This makes future maintenance harder because intent must be
inferred from surrounding logic.

Code

src/Core/gen/Eventuous.Shared.Generators/EventUsageAnalyzer.cs[R79-91]

            var m = op.TargetMethod;
+
+            // Use symbol comparison when available, fall back to string comparison
            if (m.Name != "AddType") continue;
            var ct = m.ContainingType;
            if (ct == null) continue;
-            if (ct.Name != "TypeMapper") continue;
-            var ns = ct.ContainingNamespace?.ToDisplayString();
-            if (ns != BaseNamespace) continue;
+
+            // Prefer symbol comparison (refactoring-safe)
+            var isTypeMapper = knownTypes.TypeMapper != null
+                ? SymbolEqualityComparer.Default.Equals(ct, knownTypes.TypeMapper)
+                : ct.Name == "TypeMapper" && ct.ContainingNamespace?.ToDisplayString() == BaseNamespace;
+
+            if (!isTypeMapper) continue;

Evidence

PR Compliance ID 2 requires meaningful, self-documenting identifier names and disallows
single-letter variables except conventional short-lived iterators. The highlighted code uses m and
ct for method and containing type, which are not iterators and obscure intent.

Rule 2: Generic: Meaningful Naming and Self-Documenting Code
src/Core/gen/Eventuous.Shared.Generators/EventUsageAnalyzer.cs[79-91]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Some newly modified code uses non-iterator single-letter local variable names (e.g., `m`, `ct`), which conflicts with the requirement for meaningful, self-documenting identifiers.

## Issue Context
These locals represent important semantic concepts (method symbol, containing type) and are referenced across multiple lines, so intent-revealing names improve maintainability.

## Fix Focus Areas
- src/Core/gen/Eventuous.Shared.Generators/EventUsageAnalyzer.cs[79-91]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Wrong named-arg key 🐞 Bug ✓ Correctness

Description

TypeMappingsGenerator says it checks the named argument "EventType" but it actually searches for a
member/key named "EventTypeAttribute" (the attribute class name). If EventTypeAttribute ever
supports a named argument (settable property/field), the generator will fail to read the explicit
event type name and will generate mappings without the intended name.

Code

src/Core/gen/Eventuous.Shared.Generators/TypeMappingsGenerator.cs[R134-153]

+        // Note: NamedArguments uses string keys, not symbols, so we still use string comparison here
+        // However, we verify the property exists on the attribute type when we have the symbol
+        if (eventTypeAttributeSymbol is not null) {
+            var hasProperty = eventTypeAttributeSymbol.GetMembers(EventTypeAttribute)
+                .OfType<IPropertySymbol>()
+                .Any();
+
+            if (hasProperty) {
+                foreach (var kv in attr.NamedArguments) {
+                    if (kv.Key == EventTypeAttribute && kv.Value.Value is string s) {
+                        return s;
+                    }
+                }
+            }
+        }
+
+        // Fallback to string-based comparison when symbol is not available
        foreach (var kv in attr.NamedArguments) {
            if (kv is { Key: EventTypeAttribute, Value.Value: string s }) return s;
        }

Evidence

The generator’s named-argument extraction uses Constants.EventTypeAttribute (value:
"EventTypeAttribute") both to check for a property and to match NamedArguments keys, but the actual
attribute’s member is named "EventType". This mismatch makes the named-argument path incorrect.

src/Core/gen/Eventuous.Shared.Generators/TypeMappingsGenerator.cs[125-156]
src/Core/src/Eventuous.Shared/TypeMap/TypeMapper.cs[145-148]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`TypeMappingsGenerator.TryGetEventTypeName` intends to read a named argument called `EventType`, but it checks `EventTypeAttribute` (the attribute *type* name) as the member/key. This makes the named-argument path incorrect.

## Issue Context
`EventTypeAttribute` defines a member named `EventType`, not `EventTypeAttribute`.

## Fix Focus Areas
- src/Core/gen/Eventuous.Shared.Generators/TypeMappingsGenerator.cs[125-156]
- src/Core/gen/Eventuous.Shared.Generators/Constants.cs[13-21]
- src/Core/src/Eventuous.Shared/TypeMap/TypeMapper.cs[145-148]

## Suggested approach
- Add `public const string EventTypeProperty = "EventType";` (or similar) in `Constants`.
- Update `TryGetEventTypeName` to use `EventTypeProperty` for:
 - `eventTypeAttributeSymbol.GetMembers(EventTypeProperty)`
 - `kv.Key == EventTypeProperty`
- Optionally delete the named-argument branch if it’s unnecessary (since `EventType` is currently get-only).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Missing generic arity 🐞 Bug ⛯ Reliability

Description

EventUsageAnalyzer.KnownTypeSymbols resolves
CommandHandlerBuilder/IDefineExecution/ICommandHandlerBuilder/IDefineStoreOrExecution using metadata
names without generic arity. In this repo those types are generic, so symbol resolution will fail
and the analyzer will always take the string-comparison fallback—undermining the PR’s goal of
refactoring-safe symbol comparisons for these types.

Code

src/Core/gen/Eventuous.Shared.Generators/EventUsageAnalyzer.cs[R61-67]

+            TypeMapper = compilation.GetTypeByMetadataName($"{BaseNamespace}.TypeMapper");
+            Aggregate = compilation.GetTypeByMetadataName($"{BaseNamespace}.Aggregate`1");
+            State = compilation.GetTypeByMetadataName($"{BaseNamespace}.State`1");
+            CommandHandlerBuilder = compilation.GetTypeByMetadataName($"{BaseNamespace}.CommandHandlerBuilder");
+            IDefineExecution = compilation.GetTypeByMetadataName($"{BaseNamespace}.IDefineExecution");
+            ICommandHandlerBuilder = compilation.GetTypeByMetadataName($"{BaseNamespace}.ICommandHandlerBuilder");
+            IDefineStoreOrExecution = compilation.GetTypeByMetadataName($"{BaseNamespace}.IDefineStoreOrExecution");

Evidence

KnownTypeSymbols uses GetTypeByMetadataName("Eventuous.CommandHandlerBuilder") etc. But the actual
types in Eventuous.Application are declared as generics (e.g., `CommandHandlerBuilder<TCommand,
TState>, IDefineExecution<TCommand, TState>`, and aggregate-service variants with 3 type
parameters). Without the correct metadata names (including `N arity), these cached lookups won’t
resolve and symbol-based comparisons won’t be used for functional-service Act detection.

src/Core/gen/Eventuous.Shared.Generators/EventUsageAnalyzer.cs[59-68]
src/Core/src/Eventuous.Application/FunctionalService/CommandHandlerBuilder.cs[76-127]
src/Core/src/Eventuous.Application/AggregateService/CommandHandlerBuilder.cs[94-156]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`KnownTypeSymbols` attempts to cache symbols for `CommandHandlerBuilder` and related interfaces using `GetTypeByMetadataName` with non-generic names. In this repo those types are generic, so these lookups won’t resolve and the analyzer will always use the string-based fallback.

## Issue Context
There are two sets of these types:
- Functional service variants with arity 2 (e.g., `IDefineExecution<TCommand, TState>`)
- Aggregate service variants with arity 3/4 (e.g., `IDefineExecution<TCommand, TAggregate, TState>`, `CommandHandlerBuilder<TCommand, TAggregate, TState, TId>`)

## Fix Focus Areas
- src/Core/gen/Eventuous.Shared.Generators/EventUsageAnalyzer.cs[49-69]
- src/Core/gen/Eventuous.Shared.Generators/EventUsageAnalyzer.cs[303-325]
- src/Core/src/Eventuous.Application/FunctionalService/CommandHandlerBuilder.cs[76-127]
- src/Core/src/Eventuous.Application/AggregateService/CommandHandlerBuilder.cs[94-156]

## Suggested approach
- Resolve both arities explicitly, e.g.:
 - `Eventuous.CommandHandlerBuilder`2` and `Eventuous.CommandHandlerBuilder`4`
 - `Eventuous.IDefineExecution`2` and `Eventuous.IDefineExecution`3`
 - similarly for `ICommandHandlerBuilder` and `IDefineStoreOrExecution`
- In `IsFunctionalServiceAct`, compare against `containing.OriginalDefinition` and accept matches against any resolved symbol.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[github-actions]

Test Results

0 files   -  17  0 suites   - 17   0s ⏱️ - 10m 49s
0 tests  - 272  0 ✅  - 272  0 💤 ±0  0 ❌ ±0 
0 runs   - 283  0 ✅  - 283  0 💤 ±0  0 ❌ ±0 

Results for commit 68dccb1. ± Comparison against base commit 2f9d974.