Code cleanup - #561
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f11a2503c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| file static class Events { |
There was a problem hiding this comment.
Removing TestEventHandler leaves Analyzed.cs with only two recognized event usages: State.On<TEvent> and Aggregate.Apply<TEvent>. When Should_warn_for_unannotated_events_in_state_and_aggregate runs, the analyzer therefore returns two EVTC001 diagnostics, but Analyzer_Ev001_Tests.cs:24-30 still explicitly expects at least three and identifies the now-removed EventHandler.On<TEvent> as the third, so this test deterministically fails.
Useful? React with 👍 / 👎.
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| var caller = new Thread(() => { | ||
| callerThreadId = Environment.CurrentManagedThreadId; | ||
| handler.Commit(new CommitPosition(0, 0, DateTime.UtcNow), ct).AsTask().GetAwaiter().GetResult(); | ||
| handler.Commit(new(0, 0, DateTime.UtcNow), ct).AsTask().GetAwaiter().GetResult(); | ||
| }) { IsBackground = true }; |
There was a problem hiding this comment.
3. Blocking getresult() on commit 📘 Rule violation ☼ Reliability
The modified test blocks on an async operation via .GetAwaiter().GetResult(), which violates the no-blocking-on-tasks requirement. This pattern can mask deadlocks and undermines the async-only convention in the codebase.
Agent Prompt
## Issue description
A modified test calls an async method and then blocks using `.GetAwaiter().GetResult()`.
## Issue Context
The repo compliance rule disallows blocking on tasks (e.g., `.Result`, `.Wait()`, `.GetAwaiter().GetResult()`).
## Fix Focus Areas
- src/Core/test/Eventuous.Tests.Subscriptions/CheckpointCommitHandlerConcurrencyTests.cs[51-54]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| public static SpyglassAggregateInfo? FindById(Guid id) | ||
| => Array.Find(_aggregates, x => x.Id == id); | ||
|
|
||
| public static SpyglassAggregateInfo? FindByTypeName(string typeName) { |
There was a problem hiding this comment.
4. Spyglass api method removed 🐞 Bug ⚙ Maintainability
SpyglassRegistry no longer exposes the public FindByTypeName(string) method, which is a breaking change for any external code calling it. Since Eventuous.Spyglass is a packable library, this removal changes the shipped public surface area.
Agent Prompt
## Issue description
`SpyglassRegistry.FindByTypeName(string)` was removed. This is a breaking public API change for callers that relied on lookup-by-type-name behavior.
## Issue Context
`Eventuous.Spyglass` is built as a normal (packable) library project, so removing a public method is a shipped API surface change.
## Fix Focus Areas
- src/Experimental/src/Eventuous.Spyglass/SpyglassRegistry.cs[31-53]
## Suggested fix
- Re-introduce `public static SpyglassAggregateInfo? FindByTypeName(string typeName)` with equivalent semantics to the previous implementation (including the state-suffix fallback behavior).
- If the API is intentionally being removed, add an alternative replacement API and keep `FindByTypeName` as an `[Obsolete]` forwarder for at least one release (or ensure this ships only with a major version bump + migration notes).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
PR Summary by QodoModernize codebase with C# collection expressions and improved async cleanup
AI Description
Diagram
High-Level Assessment
Files changed (72)
|
There was a problem hiding this comment.
Pull request overview
Modernizes and tidies code across Eventuous (core + integrations + tests) by adopting newer C# features (collection expressions, target-typed new, extension blocks) and simplifying a few implementations, largely replacing ToArray()/ToList() and some legacy conditional compilation.
Changes:
- Replaced many
.ToArray()/.ToList()calls with collection expressions ([..],[.. a, .. b]) and minor target-typednewcleanups. - Small behavioral/implementation refactors (e.g., reflection helper loop, async dispose usage in KurrentDB subscription).
- Removed/adjusted some code and test helper snippets as part of cleanup.
Reviewed changes
Copilot reviewed 70 out of 72 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Eventuous.TestHelpers/TestHelper.cs | Refactors member lookup recursion into an iterative loop. |
| test/Eventuous.TestHelpers.TUnit/TestEventListener.cs | Uses collection expression when formatting event payload arguments. |
| src/Testing/src/Eventuous.Testing/InMemoryEventStore.cs | Adds suppression comment for async iterator without await. |
| src/Sqlite/test/Eventuous.Tests.Sqlite/Subscriptions/SubscribeTests.cs | Uses collection expressions in assertions and append calls. |
| src/Sqlite/test/Eventuous.Tests.Sqlite/Store/StoreFixture.cs | Simplifies fixture declaration (removes primary-ctor syntax). |
| src/SignalR/test/Eventuous.Tests.SignalR/TypedStreamSubscriptionTests.cs | Formatting cleanup of local variable alignment. |
| src/SignalR/src/Eventuous.SignalR.Client/SignalRSubscriptionClient.cs | Target-typed exception creation in channel completion. |
| src/Redis/test/Eventuous.Tests.Redis/Subscriptions/SubscribeToStream.cs | Uses collection expression when appending stream events. |
| src/Redis/test/Eventuous.Tests.Redis/Subscriptions/SubscribeToAll.cs | Uses collection expression when appending stream events. |
| src/Redis/test/Eventuous.Tests.Redis/Store/Helpers.cs | Uses collection expression when appending stream events. |
| src/Redis/src/Eventuous.Redis/Subscriptions/RedisStreamSubscription.cs | Converts LINQ .ToArray() to collection expression array creation. |
| src/Redis/src/Eventuous.Redis/Subscriptions/RedisAllStreamSubscription.cs | Converts .ToArray() to collection expression. |
| src/Redis/src/Eventuous.Redis/RedisStore.cs | Converts .ToArray() to collection expression. |
| src/Postgres/test/Eventuous.Tests.Postgres/Subscriptions/TombstonesCreationTest.cs | Uses collection expressions for fixture event appends. |
| src/Postgres/test/Eventuous.Tests.Postgres/Subscriptions/GapIgnoreTest.cs | Uses collection expressions for fixture event appends. |
| src/Postgres/test/Eventuous.Tests.Postgres/Store/TieredStoreTests.cs | Adds suppression comment for ReSharper unused type. |
| src/Postgres/src/Eventuous.Postgresql/Projections/PostgresProjector.cs | Uses collection expression instead of ToArray() for parameters. |
| src/KurrentDB/test/Eventuous.Tests.KurrentDB/Subscriptions/ResolvedLinkCheckpointTests.cs | Target-typed new for EventData creation. |
| src/KurrentDB/test/Eventuous.Tests.KurrentDB/Subscriptions/PersistentSubscriptionFailureTests.cs | Target-typed new for subscription settings creation. |
| src/KurrentDB/test/Eventuous.Tests.KurrentDB/Subscriptions/CheckpointReachedTests.cs | Uses collection expression for list creation. |
| src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/AllStreamSubscription.cs | Switches to async disposal in multiple paths. |
| src/KurrentDB/src/Eventuous.KurrentDB/KurrentDBEventStore.cs | Minor cleanup (async iterator suppression; collection expression). |
| src/Extensions/test/Eventuous.Tests.Extensions.AspNetCore/DiscoveredCommandsTests.cs | Uses collection expression in endpoint name collection. |
| src/Extensions/test/Eventuous.Tests.Extensions.AspNetCore.Analyzers/AnalyzerTestShared.cs | Uses collection expression for diagnostic filtering result. |
| src/Extensions/src/Eventuous.Extensions.AspNetCore/Http/CommandMappingRegistry.cs | Uses collection expression under lock instead of ToList(). |
| src/Experimental/test/Eventuous.Tests.Spyglass.Generators/CompilationHelper.cs | Uses collection expression for diagnostics materialization. |
| src/Experimental/src/Eventuous.Spyglass/SpyglassRegistry.cs | Refactors backing store fields; uses Lock alias; changes available lookup helpers. |
| src/Experimental/src/Eventuous.ElasticSearch/Store/ElasticEventStore.cs | Converts .ToArray() to collection expression array creation. |
| src/Experimental/src/Eventuous.ElasticSearch/Producers/ElasticProducer.cs | Converts .ToList() to collection expression list creation. |
| src/Experimental/src/ElasticPlayground/Generator.cs | Makes generator class static. |
| src/Diagnostics/test/Eventuous.Tests.OpenTelemetry/Fakes/TestExporter.cs | Uses collection expressions for arrays and return value. |
| src/Diagnostics/src/Eventuous.Diagnostics.Logging/LoggingEventListener.cs | Uses collection expression for payload args. |
| src/Core/test/Eventuous.Tests/Fixtures/IdGenerator.cs | Makes ID generator class static. |
| src/Core/test/Eventuous.Tests.Subscriptions/ResubscribeOnHandlerFailureTests.cs | Simplifies TaskRunner delegate usage. |
| src/Core/test/Eventuous.Tests.Subscriptions/CheckpointCommitHandlerConcurrencyTests.cs | Target-typed new for commit position creation. |
| src/Core/test/Eventuous.Tests.Subscriptions/CheckpointCommitHandlerBackpressureTests.cs | Uses collection expressions for snapshots and expectations. |
| src/Core/test/Eventuous.Tests.Subscriptions.Base/SubscriptionMeasureBase.cs | Removes unused using directive. |
| src/Core/test/Eventuous.Tests.Subscriptions.Base/SubscriptionDropBase.cs | Simplifies predicate by method group usage. |
| src/Core/test/Eventuous.Tests.Subscriptions.Base/SubscribeToStream.cs | Uses collection expression when appending stream events. |
| src/Core/test/Eventuous.Tests.Subscriptions.Base/Fixtures/TestEventHandler.cs | Uses collection expression for concurrent-queue snapshot. |
| src/Core/test/Eventuous.Tests.Shared.Analyzers/Analyzer_Ev001_Tests.cs | Uses collection expression; simplifies reference loading loop. |
| src/Core/test/Eventuous.Tests.Shared.Analyzers/Analyzed.cs | Removes a file-local TestEventHandler snippet. |
| src/Core/test/Eventuous.Tests.Persistence.Base/Store/Read.cs | Uses collection expressions for created event arrays. |
| src/Core/test/Eventuous.Tests.Persistence.Base/Fixtures/StoreFixtureBase.cs | Removes non-NET8 fallback for GeneratedRegex. |
| src/Core/test/Eventuous.Tests.Persistence.Base/Fixtures/Helpers.cs | Uses collection expression when appending stream events. |
| src/Core/test/Eventuous.Tests.Application/ServiceTestBase.Amendments.cs | Adjusts test to ignore Handle result (currently via unused local). |
| src/Core/src/Eventuous.Subscriptions/SubscriptionHostedService.cs | Removes AOT suppression attributes and related using. |
| src/Core/src/Eventuous.Subscriptions/Registrations/SubscriptionBuilder.cs | Uses collection expression for resolving handler array. |
| src/Core/src/Eventuous.Subscriptions/Filters/TracingFilter.cs | Uses collection expression concat for default tags. |
| src/Core/src/Eventuous.Subscriptions/Filters/PartitioningFilter.cs | Uses collection expression for filter array creation. |
| src/Core/src/Eventuous.Subscriptions/Diagnostics/SubscriptionMetrics.cs | Uses collection expression concat for custom tags. |
| src/Core/src/Eventuous.Subscriptions/Channels/ChannelWorkerBase.cs | Uses collection expression for reader tasks; removes NET8 conditional cancel path. |
| src/Core/src/Eventuous.Shared/TypeMap/TypeMapper.cs | Uses collection expression for assembly list materialization. |
| src/Core/src/Eventuous.Shared/Tools/TaskRunner.cs | Removes NET8 conditional; always uses CancelAsync. |
| src/Core/src/Eventuous.Shared/Tools/TaskExtensions.cs | Uses collection expression; removes older NoThrow fallback implementation. |
| src/Core/src/Eventuous.Shared/Tools/Ensure.cs | Removes older NotEmptyString fallback implementation. |
| src/Core/src/Eventuous.Producers/BaseProducer.cs | Uses collection expression concat for tags. |
| src/Core/src/Eventuous.Persistence/EventStore/StoreFunctions.cs | Uses extension blocks + collection expressions for materialization. |
| src/Core/src/Eventuous.Persistence/Diagnostics/Tracing/TracedEventWriter.cs | Simplifies traced append construction; uses collection expressions internally. |
| src/Core/src/Eventuous.Persistence/Diagnostics/Tracing/TracedEventReader.cs | Passes cancellation token through to TraceEnumerable. |
| src/Core/src/Eventuous.Persistence/Diagnostics/Tracing/BaseTracer.cs | Signature alignment/formatting update for TraceEnumerable. |
| src/Core/src/Eventuous.Persistence/Diagnostics/PersistenceMetrics.cs | Uses collection expression for TagList materialization. |
| src/Core/src/Eventuous.Domain/Aggregate.cs | Uses collection expression for Original event materialization in Load. |
| src/Core/src/Eventuous.Diagnostics/EventuousDiagnostics.cs | Uses collection expression when updating global Tags. |
| src/Core/src/Eventuous.Application/Persistence/WriterExtensions.cs | Uses collection expression for event materialization. |
| src/Core/src/Eventuous.Application/Diagnostics/CommandServiceMetrics.cs | Uses collection expression for TagList materialization. |
| src/Core/src/Eventuous.Application/AggregateService/CommandService.cs | Uses collection expression for proposed events materialization. |
| src/Core/gen/Eventuous.Subscriptions.Generators/ConsumeContextConverterGenerator.cs | Adds explicit nullable type annotation in loop variable. |
| src/Core/gen/Eventuous.Shared.Generators/EventUsageAnalyzer.cs | Refactors switch case by removing an explicit block body. |
| src/Benchmarks/Benchmarks/ChannelBatchingBenchmarks.cs | Uses collection expression; simplifies CollectionsMarshal reference. |
| src/Azure/test/Eventuous.Tests.Azure.ServiceBus/ConvertEventToMessage.cs | Target-typed new for DateTimeOffset creation. |
| samples/kurrentdb/Bookings/HttpApi/Bookings/CommandApiWithCustomResult.cs | Uses collection expression for validation error messages. |
Suppressed comments (2)
src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/AllStreamSubscription.cs:209
- Same as above: the finally block uses .NoContext() for messages.DisposeAsync(), but not for subscription.DisposeAsync(). For consistency with the rest of the file and to avoid synchronization-context capture, apply .NoContext() here too.
src/KurrentDB/src/Eventuous.KurrentDB/Subscriptions/AllStreamSubscription.cs:231 - Unsubscribe is also library code and elsewhere uses .NoContext() for awaits. Applying .NoContext() to _subscription.DisposeAsync() keeps the pattern consistent and avoids capturing a context during shutdown.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var cmd = CreateCommand(); | ||
|
|
||
| var result = await service.Handle(cmd, cancellationToken); | ||
| var dummy = await service.Handle(cmd, cancellationToken); |
| public void Load(long version, IEnumerable<object?> events) { | ||
| Original = events.Where(x => x != null).ToArray()!; | ||
| Original = [.. events.Where(x => x != null)!]; | ||
| OriginalVersion = version; |
| static SpyglassAggregateInfo[] aggregates = []; | ||
| static readonly Lock Lock = new(); | ||
|
|
||
| public static void Register(SpyglassAggregateInfo info) { | ||
| lock (_lock) { | ||
| lock (Lock) { | ||
| var entry = info with { Id = Guid.NewGuid() }; |
| public static SpyglassAggregateEntry[] GetAggregates() | ||
| => _aggregates.Select(a => new SpyglassAggregateEntry(a.Id, a.AggregateType, a.StateType, a.Methods, a.Events)).ToArray(); | ||
| => [.. aggregates.Select(a => new SpyglassAggregateEntry(a.Id, a.AggregateType, a.StateType, a.Methods, a.Events))]; | ||
|
|
||
| public static SpyglassAggregateInfo? FindById(Guid id) | ||
| => Array.Find(_aggregates, x => x.Id == id); | ||
|
|
||
| public static SpyglassAggregateInfo? FindByTypeName(string typeName) { | ||
| var snapshot = _aggregates; | ||
|
|
||
| return Array.Find(snapshot, x => x.AggregateType == typeName) | ||
| ?? Array.Find(snapshot, x => StripStateSuffix(x.StateType) == typeName); | ||
| } | ||
|
|
||
| static string StripStateSuffix(string s) | ||
| => s.EndsWith("State") && s.Length > 5 ? s[..^5] : s; | ||
| => Array.Find(aggregates, x => x.Id == id); |
| } catch { | ||
| await messages.DisposeAsync().NoContext(); | ||
| subscription.Dispose(); | ||
| await subscription.DisposeAsync(); | ||
|
|
Test Results 28 files + 6 28 suites +6 10m 36s ⏱️ - 3m 11s For more details on these failures, see this check. Results for commit c4c63bc. ± Comparison against base commit 37402bc. This pull request removes 67 and adds 9 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 73 out of 75 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/Core/test/Eventuous.Tests.Subscriptions/CheckpointCommitHandlerBackpressureTests.cs:162
- This second
return;also leaves the local function declaration unreachable, producing an "unreachable code" warning. Remove thereturn;so the method ends naturally.
src/Experimental/src/Eventuous.Spyglass/SpyglassRegistry.cs:53 SpyglassRegistry.FindByTypeNamewas removed. Since this is a public API surface, removing it is a breaking change for existing consumers; if this wasn't intentional, consider restoring it (or marking it obsolete first) to preserve compatibility.
src/Core/test/Eventuous.Tests.Application/ServiceTestBase.Amendments.cs:13dummyis assigned but never used. If the intent is just to execute the command, discard the result explicitly to avoid unused-variable warnings.
var cmd = CreateCommand();
var dummy = await service.Handle(cmd, cancellationToken);
src/Core/test/Eventuous.Tests.Subscriptions/CheckpointCommitHandlerBackpressureTests.cs:72
- The
return;statement makes the subsequent local function declaration unreachable, which triggers an "unreachable code" warning and can fail the build if warnings are treated as errors. Remove thereturn;and let the method fall through to the local function declaration.
This issue also appears on line 157 of the same file.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 90 out of 92 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
src/Core/test/Eventuous.Tests.Subscriptions/CheckpointCommitHandlerBackpressureTests.cs:159
- This
return;is redundant and makes the test harder to follow because it separates the test body from the localCommitFnused to construct the handler. Removing it improves readability.
src/Experimental/src/Eventuous.Spyglass/SpyglassRegistry.cs:53 SpyglassRegistrypreviously exposed lookup by type name; removing it is a breaking change for any callers that relied on it (even if internal usage is currently none). If the intent is just refactoring, consider reintroducingFindByTypeName(and its helper) to preserve the public surface area.
src/Core/test/Eventuous.Tests.Subscriptions/CheckpointCommitHandlerBackpressureTests.cs:72- This
return;is redundant and makes the test harder to read because the localCommitFnthat the handler uses is declared after an early-exit statement. Removing it improves clarity without changing behavior.
This issue also appears on line 159 of the same file.
src/RabbitMq/src/Eventuous.RabbitMq/Producers/RabbitMqProducer.cs:77
Publish(...)is invoked to create Tasks that start executing immediately, so this loop can initiate multiple concurrent publishes against the same_channel. RabbitMQ channels are typically not safe for concurrent use; even if they are, this pattern makes ordering and failure handling harder to reason about. Prefer deferring task creation (store delegates) or publishing sequentially.
| public async Task EnsureExchange(string name, Func<Task> createExchange) { | ||
| if (_exchanges.Contains(name)) return; |
No description provided.