Issue/736 add more warnings as errors#11
Merged
Conversation
- Set latest analysis level and adjust warning settings in Directory.Build.props - Add Scalar API reference and improve exception handling in MiddlewareConfig - Standardize cancellation token usage in endpoints - Pass cancellation token to Task.Delay in NoOpMediator
Changed DisposeAsync in CustomWebApplicationFactory from new to override, added call to base.DisposeAsync, and suppressed finalization to ensure proper resource disposal and .NET best practices.
Ensure all int-to-string conversions for routes and logging use CultureInfo.InvariantCulture for consistent, culture-agnostic output across LoggerConfigs and contributor request files. This improves reliability in globalized scenarios.
Changed Constants to static and renamed fields to PascalCase. Updated all usages to match new names across queries, endpoints, DTOs, and validation to align with .NET standards.
Changed structured logging placeholders from lowercase to PascalCase across multiple files to improve consistency and align with .NET logging conventions.
Renamed constants to PascalCase for .NET convention compliance, updated all references, and removed an unused FastEndpoints using directive.
Changed entity filtering to use DomainEvents.Count != 0 instead of DomainEvents.Any() when dispatching events. This is a minor refactor with no functional impact.
- Use PascalCase {ContributorId} in logging for consistency
- Refactor BaseEfRepoTestFixture: private readonly _dbContext, add protected DbContext property
- Implement IDisposable and IAsyncDisposable for proper DbContext disposal
- Update EfRepositoryUpdate to use new DbContext property
… warning brcaus to is a reserfved word
CA1014 (CLS compliance) is now disabled by setting its severity to none, clarifying that CLS compliance is not required for this application or template repository.
Expanded and reorganized .editorconfig for C#/VB conventions and .NET rules. Updated .gitignore for SonarQube and VS artifacts. Modified Clean.Architecture.slnx to include .gitignore and adjust test config references. Added .runsettings for parallel xUnit test execution.
.editorconfig updated to suppress CA1014 and CA1707, and set CA1848 as a suggestion. Refactored DeleteContributorService to use a static LoggerMessage.Define delegate for contributor deletion logs, replacing direct LogInformation calls. No functional changes made.
The PhoneNumber property in CreateContributorRequest is now a nullable string without an explicit default value assignment. This simplifies the property declaration.
Updated DeleteContributorService summary and remarks to explain logging approach. Method now returns Result.Success() after deletion and event publication.
Update GetContributorQuery and related files to use the new Clean.Architecture.UseCases.Contributors.GetContributor namespace. Adjust using directives and references in handler and endpoint files for consistency.
Renamed ct to cancellationToken in UpdateContributorHandler for clarity and consistency. Changed NoOpMediator.CreateStream to a static method.
.editorconfig updated to disable CA1848 and CA1873 rules and clarify LoggerMessage usage is not enforced. Solution file now includes .editorconfig in /src/ to apply coding conventions and analyzer settings to all source projects.
Seal VogenEfCoreConverters class for stricter encapsulation Changed VogenEfCoreConverters from internal partial to internal sealed partial, preventing inheritance and enforcing stricter encapsulation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughCoordinated repository-wide updates: editor and analysis settings, renamed constants and parameters to PascalCase, logging-placeholder casing, culture-invariant route/formatting, constructor/handler parameter normalization, disposal/cancellation fixes in tests and endpoints. ChangesNaming Convention & Code Quality Improvements
Culture-Aware Route Building & Logging
Code Quality & Sealing
Namespace, Import, and Minor Cleanups
Consumer-Side Updates for Renamed Constants
Cancellation & Disposal Pattern Updates
Build Configuration & Project Structure
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Clean.Architecture.UnitTests/NoOpMediator.cs (1)
56-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
IStreamQueryinterface path bypasses the new cancellation-aware logic.On Lines 56-59,
ISender.CreateStream<TResponse>(IStreamQuery<TResponse> ...)still returns immediately and never uses the new method on Lines 5-8. Any caller going throughIMediator/ISenderwon’t observe the intended cancellation-aware behavior.Suggested fix
IAsyncEnumerable<TResponse> ISender.CreateStream<TResponse>(IStreamQuery<TResponse> query, CancellationToken cancellationToken) { - return AsyncEnumerable.Empty<TResponse>(); + return CreateStream(query, cancellationToken).GetAwaiter().GetResult(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Clean.Architecture.UnitTests/NoOpMediator.cs` around lines 56 - 59, The ISender.CreateStream<TResponse>(IStreamQuery<TResponse> query, CancellationToken cancellationToken) implementation currently ignores cancellation and returns AsyncEnumerable.Empty, bypassing the new cancellation-aware logic; update this method to delegate to the cancellation-aware CreateStream overload instead of returning empty: call the class's cancellation-aware CreateStream<TResponse>(IStreamQuery<TResponse> query, CancellationToken cancellationToken) (or the internal method introduced on lines 5-8) and pass through the provided CancellationToken, or if the IStreamQuery<TResponse> exposes its own CancellationToken property use that, otherwise forward CancellationToken.None — this ensures callers via IMediator/ISender observe the intended cancellation behavior.
🧹 Nitpick comments (3)
tests/Clean.Architecture.UnitTests/NoOpMediator.cs (1)
5-5: ⚡ Quick winAsync method name should end with
Async.On Line 5,
CreateStreamisasyncbut missing theAsyncsuffix (CreateStreamAsync), which breaks the repository’s C# naming rule.As per coding guidelines, "Use 'Async' suffix for async methods (e.g., GetByIdAsync)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Clean.Architecture.UnitTests/NoOpMediator.cs` at line 5, The async method CreateStream should be renamed to CreateStreamAsync to follow the Async-suffix convention; update the method declaration in NoOpMediator (public static async Task<IAsyncEnumerable<TResponse>> CreateStream<TResponse>(...)) to CreateStreamAsync<TResponse>(...) and then update all callers, tests, and any interface references (e.g., usages of IStreamQuery or test helpers) to call CreateStreamAsync; ensure the method signature, generic type parameter, CancellationToken parameter, and async behavior remain unchanged so functionality is preserved.Source: Coding guidelines
src/Clean.Architecture.Infrastructure/Email/FakeEmailSender.cs (1)
8-10: ⚡ Quick winLog placeholder names should match parameter names for structured logging clarity.
The log template uses
{To},{From}, and{Subject}, but the actual parameter names arerecipientEmail,senderEmail, andsubject. In structured logging, placeholder names become property names in the output, so they should match the source variable names for consistency and clarity.📝 Suggested fix to align placeholders with parameter names
- public Task SendEmailAsync(string recipientEmail, string senderEmail, string subject, string body) - { - _logger.LogInformation("Not actually sending an email to {To} from {From} with subject {Subject}", recipientEmail, senderEmail, subject); + public Task SendEmailAsync(string recipientEmail, string senderEmail, string subject, string body) + { + _logger.LogInformation("Not actually sending an email to {RecipientEmail} from {SenderEmail} with subject {Subject}", recipientEmail, senderEmail, subject);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Clean.Architecture.Infrastructure/Email/FakeEmailSender.cs` around lines 8 - 10, Update the structured log in FakeEmailSender.SendEmailAsync to use placeholder names that match the method parameters: replace "{To}", "{From}", and "{Subject}" in the _logger.LogInformation call with "{recipientEmail}", "{senderEmail}", and "{subject}" respectively so the emitted log property names align with the actual variables.src/Clean.Architecture.Infrastructure/Email/MimeKitEmailSender.cs (1)
11-13: ⚡ Quick winLog placeholder names should match parameter names for structured logging clarity.
The log template uses
{To},{From}, and{Subject}, but the actual parameter names arerecipientEmail,senderEmail, andsubject. For consistency with structured logging best practices and the patterns used elsewhere in this PR (e.g.,DeleteContributorService,ContributorDeletedHandler), placeholder names should match the source variable names.📝 Suggested fix to align placeholders with parameter names
- public async Task SendEmailAsync(string recipientEmail, string senderEmail, string subject, string body) - { - _logger.LogWarning("Sending email to {To} from {From} with subject {Subject} using {Type}.", recipientEmail, senderEmail, subject, this.ToString()); + public async Task SendEmailAsync(string recipientEmail, string senderEmail, string subject, string body) + { + _logger.LogWarning("Sending email to {RecipientEmail} from {SenderEmail} with subject {Subject} using {Type}.", recipientEmail, senderEmail, subject, this.ToString());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Clean.Architecture.Infrastructure/Email/MimeKitEmailSender.cs` around lines 11 - 13, In SendEmailAsync, update the structured log template so its placeholders match the actual parameter names: use {recipientEmail}, {senderEmail} and {subject} (and keep {Type} or this.ToString() if desired) instead of {To}, {From}, {Subject}; modify the _logger.LogWarning call in MimeKitEmailSender.SendEmailAsync to use those parameter-aligned placeholders so the logged properties bind correctly to recipientEmail, senderEmail and subject.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Clean.Architecture.slnx`:
- Line 16: The solution contains a solution item entry with File
Path=".editorconfig" that incorrectly references the repository root config;
update that solution item so its File Path points to the src-specific file
(change the value to "src/.editorconfig") so the solution references the src
folder's editorconfig instead of the root one, ensuring the solution wiring and
IDE view show the per-source configuration correctly.
In `@Directory.Build.props`:
- Line 8: The project currently sets the MSBuild property
CodeAnalysisTreatWarningsAsErrors to false which disables treating analyzer
diagnostics as build-breaking; change the value of the
CodeAnalysisTreatWarningsAsErrors property in Directory.Build.props to true so
analyzer warnings are treated as errors (or make it conditionally true for CI
builds if you need local leniency), ensuring analyzer findings fail the build
and restore the intended quality gate.
In `@src/Clean.Architecture.UseCases/Contributors/Get/GetContributorHandler.cs`:
- Line 10: GetContributorHandler currently takes IReadRepository<Contributor>
repository as a primary constructor parameter but uses it directly; create a
private readonly field named _repository and assign the constructor parameter to
it in the constructor (e.g., _repository = repository) and then replace direct
uses of repository (e.g., in the Handle method) with _repository to follow the
guideline of assigning primary constructor parameters to private fields for
clarity and testability.
---
Outside diff comments:
In `@tests/Clean.Architecture.UnitTests/NoOpMediator.cs`:
- Around line 56-59: The ISender.CreateStream<TResponse>(IStreamQuery<TResponse>
query, CancellationToken cancellationToken) implementation currently ignores
cancellation and returns AsyncEnumerable.Empty, bypassing the new
cancellation-aware logic; update this method to delegate to the
cancellation-aware CreateStream overload instead of returning empty: call the
class's cancellation-aware CreateStream<TResponse>(IStreamQuery<TResponse>
query, CancellationToken cancellationToken) (or the internal method introduced
on lines 5-8) and pass through the provided CancellationToken, or if the
IStreamQuery<TResponse> exposes its own CancellationToken property use that,
otherwise forward CancellationToken.None — this ensures callers via
IMediator/ISender observe the intended cancellation behavior.
---
Nitpick comments:
In `@src/Clean.Architecture.Infrastructure/Email/FakeEmailSender.cs`:
- Around line 8-10: Update the structured log in FakeEmailSender.SendEmailAsync
to use placeholder names that match the method parameters: replace "{To}",
"{From}", and "{Subject}" in the _logger.LogInformation call with
"{recipientEmail}", "{senderEmail}", and "{subject}" respectively so the emitted
log property names align with the actual variables.
In `@src/Clean.Architecture.Infrastructure/Email/MimeKitEmailSender.cs`:
- Around line 11-13: In SendEmailAsync, update the structured log template so
its placeholders match the actual parameter names: use {recipientEmail},
{senderEmail} and {subject} (and keep {Type} or this.ToString() if desired)
instead of {To}, {From}, {Subject}; modify the _logger.LogWarning call in
MimeKitEmailSender.SendEmailAsync to use those parameter-aligned placeholders so
the logged properties bind correctly to recipientEmail, senderEmail and subject.
In `@tests/Clean.Architecture.UnitTests/NoOpMediator.cs`:
- Line 5: The async method CreateStream should be renamed to CreateStreamAsync
to follow the Async-suffix convention; update the method declaration in
NoOpMediator (public static async Task<IAsyncEnumerable<TResponse>>
CreateStream<TResponse>(...)) to CreateStreamAsync<TResponse>(...) and then
update all callers, tests, and any interface references (e.g., usages of
IStreamQuery or test helpers) to call CreateStreamAsync; ensure the method
signature, generic type parameter, CancellationToken parameter, and async
behavior remain unchanged so functionality is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3780c851-e071-4ba9-9695-5930c16c4d01
📒 Files selected for processing (38)
.editorconfig.gitignoreClean.Architecture.slnxDirectory.Build.propssrc/Clean.Architecture.Core/ContributorAggregate/Handlers/ContributorDeletedHandler.cssrc/Clean.Architecture.Core/ContributorAggregate/Handlers/ContributorNameUpdatedEmailNotificationHandler.cssrc/Clean.Architecture.Core/Interfaces/IEmailSender.cssrc/Clean.Architecture.Core/Services/DeleteContributorService.cssrc/Clean.Architecture.Infrastructure/Data/Config/DataSchemaConstants.cssrc/Clean.Architecture.Infrastructure/Data/Config/VogenEfCoreConverters.cssrc/Clean.Architecture.Infrastructure/Data/EventDispatcherInterceptor.cssrc/Clean.Architecture.Infrastructure/Data/SeedData.cssrc/Clean.Architecture.Infrastructure/Email/FakeEmailSender.cssrc/Clean.Architecture.Infrastructure/Email/MimeKitEmailSender.cssrc/Clean.Architecture.ServiceDefaults/Extensions.cssrc/Clean.Architecture.UseCases/Constants.cssrc/Clean.Architecture.UseCases/Contributors/Get/GetContributorHandler.cssrc/Clean.Architecture.UseCases/Contributors/GetContributor/GetContributorQuery.cssrc/Clean.Architecture.UseCases/Contributors/List/ListContributorsHandler.cssrc/Clean.Architecture.UseCases/Contributors/List/ListContributorsQuery.cssrc/Clean.Architecture.UseCases/Contributors/Update/UpdateContributorHandler.cssrc/Clean.Architecture.Web/Configurations/LoggerConfigs.cssrc/Clean.Architecture.Web/Configurations/MiddlewareConfig.cssrc/Clean.Architecture.Web/Contributors/Create.cssrc/Clean.Architecture.Web/Contributors/Delete.DeleteContributorRequest.cssrc/Clean.Architecture.Web/Contributors/GetById.GetContributorByIdRequest.cssrc/Clean.Architecture.Web/Contributors/GetById.cssrc/Clean.Architecture.Web/Contributors/List.cssrc/Clean.Architecture.Web/Contributors/Update.UpdateContributorRequest.cssrc/Clean.Architecture.Web/Contributors/Update.UpdateContributorValidator.cssrc/Clean.Architecture.Web/Contributors/Update.cstests/.editorconfigtests/.runsettingstests/Clean.Architecture.FunctionalTests/ApiEndpoints/ContributorList.cstests/Clean.Architecture.FunctionalTests/CustomWebApplicationFactory.cstests/Clean.Architecture.IntegrationTests/Data/BaseEfRepoTestFixture.cstests/Clean.Architecture.IntegrationTests/Data/EfRepositoryUpdate.cstests/Clean.Architecture.UnitTests/NoOpMediator.cs
💤 Files with no reviewable changes (1)
- src/Clean.Architecture.Web/Contributors/Update.cs
| <File Path="Directory.Packages.props" /> | ||
| </Folder> | ||
| <Folder Name="/src/"> | ||
| <File Path=".editorconfig" /> |
There was a problem hiding this comment.
Wrong solution item path under /src/ folder.
On Line 16, File Path=".editorconfig" points to the repository root file again, not src/.editorconfig. This breaks the intended solution wiring for src-specific config and can mislead contributors in IDE solution view.
Suggested fix
- <File Path=".editorconfig" />
+ <File Path="src/.editorconfig" />📝 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.
Suggested change
| <File Path=".editorconfig" /> | |
| <File Path="src/.editorconfig" /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Clean.Architecture.slnx` at line 16, The solution contains a solution item
entry with File Path=".editorconfig" that incorrectly references the repository
root config; update that solution item so its File Path points to the
src-specific file (change the value to "src/.editorconfig") so the solution
references the src folder's editorconfig instead of the root one, ensuring the
solution wiring and IDE view show the per-source configuration correctly.
| <AnalysisLevel>latest</AnalysisLevel> | ||
| <AnalysisMode>Recommended</AnalysisMode> | ||
| <TreatWarningsAsErrors>true</TreatWarningsAsErrors> | ||
| <CodeAnalysisTreatWarningsAsErrors>false</CodeAnalysisTreatWarningsAsErrors> |
There was a problem hiding this comment.
Analyzer warnings are no longer enforced as errors.
On Line 8, setting CodeAnalysisTreatWarningsAsErrors to false creates a policy gap: compiler warnings fail builds, but analyzer findings won’t. That undercuts this PR’s quality gate objective and can let new analyzer regressions merge unnoticed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Directory.Build.props` at line 8, The project currently sets the MSBuild
property CodeAnalysisTreatWarningsAsErrors to false which disables treating
analyzer diagnostics as build-breaking; change the value of the
CodeAnalysisTreatWarningsAsErrors property in Directory.Build.props to true so
analyzer warnings are treated as errors (or make it conditionally true for CI
builds if you need local leniency), ensuring analyzer findings fail the build
and restore the intended quality gate.
| /// Queries don't necessarily need to use repository methods, but they can if it's convenient | ||
| /// </summary> | ||
| public class GetContributorHandler(IReadRepository<Contributor> _repository) | ||
| public class GetContributorHandler(IReadRepository<Contributor> repository) |
There was a problem hiding this comment.
Assign the primary constructor parameter to a private field.
The guideline explicitly states: "Never use primary constructor parameters directly - always assign to private fields for clarity and testability." The parameter repository is used directly in line 16 without being assigned to a private field.
As per coding guidelines, use a private field with underscore prefix.
🔧 Proposed fix to add private field assignment
-public class GetContributorHandler(IReadRepository<Contributor> repository)
+public class GetContributorHandler(IReadRepository<Contributor> repository)
: IQueryHandler<GetContributorQuery, Result<ContributorDto>>
{
+ private readonly IReadRepository<Contributor> _repository = repository;
+
public async ValueTask<Result<ContributorDto>> Handle(GetContributorQuery request, CancellationToken cancellationToken)
{
var spec = new ContributorByIdSpec(request.ContributorId);
- var entity = await repository.FirstOrDefaultAsync(spec, cancellationToken);
+ var entity = await _repository.FirstOrDefaultAsync(spec, cancellationToken);📝 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.
Suggested change
| public class GetContributorHandler(IReadRepository<Contributor> repository) | |
| public class GetContributorHandler(IReadRepository<Contributor> repository) | |
| : IQueryHandler<GetContributorQuery, Result<ContributorDto>> | |
| { | |
| private readonly IReadRepository<Contributor> _repository = repository; | |
| public async ValueTask<Result<ContributorDto>> Handle(GetContributorQuery request, CancellationToken cancellationToken) | |
| { | |
| var spec = new ContributorByIdSpec(request.ContributorId); | |
| var entity = await _repository.FirstOrDefaultAsync(spec, cancellationToken); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Clean.Architecture.UseCases/Contributors/Get/GetContributorHandler.cs` at
line 10, GetContributorHandler currently takes IReadRepository<Contributor>
repository as a primary constructor parameter but uses it directly; create a
private readonly field named _repository and assign the constructor parameter to
it in the constructor (e.g., _repository = repository) and then replace direct
uses of repository (e.g., in the Handle method) with _repository to follow the
guideline of assigning primary constructor parameters to private fields for
clarity and testability.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
reduced warnings
Summary by CodeRabbit
Chores
Refactor
Tests