-
-
Notifications
You must be signed in to change notification settings - Fork 41
feat: Enable null reference types #239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/AzureEventGridSimulator/Domain/Services/Delivery/EventHubEventDeliveryService.cs
Fixed
Show fixed
Hide fixed
src/AzureEventGridSimulator/Infrastructure/Middleware/EventGridMiddleware.cs
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enables nullable reference types across the entire codebase, a significant modernization effort that improves type safety and eliminates many potential null reference errors. The changes include:
- Enabled nullable reference types (
<Nullable>enable</Nullable>) and treat warnings as errors - Added Meziantou.Analyzer for enhanced code quality checks
- Converted mutable properties to
init-only and addedrequiredmodifiers where appropriate - Updated all nullable annotations throughout the codebase
- Converted several classes to record types for immutability
- Added proper
StringComparisonparameters to string operations - Updated error messages to be more descriptive and schema-specific
- Enhanced validation and null-safety throughout
Reviewed changes
Copilot reviewed 97 out of 97 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Directory.Build.props | Enabled nullable reference types and warnings as errors, added Meziantou.Analyzer |
| Directory.Packages.props | Added Meziantou.Analyzer package reference |
| Settings classes | Converted properties to init-only with required modifiers and nullable annotations |
| EventGridEvent/CloudEvent | Added required modifiers, improved validation messages |
| Middleware classes | Enhanced error handling with detailed error codes and null-safe operations |
| Service classes | Added nullable annotations and proper null checks |
| Record types | Converted DTOs to records for immutability |
| Test files | Updated to use required properties and null-aware assertions |
| .editorconfig | Added Meziantou.Analyzer rule configurations |
The changes are comprehensive and well-executed, successfully modernizing the codebase to use nullable reference types throughout.
...dSimulator.Tests/UnitTests/Commands/SendNotificationEventsToSubscriberCommandHandlerTests.cs
Outdated
Show resolved
Hide resolved
src/AzureEventGridSimulator/Domain/Services/Delivery/EventHubEventDeliveryService.cs
Outdated
Show resolved
Hide resolved
- Restructure null check in EventHubEventDeliveryService to avoid redundant condition - Inline errorMessage variable in EventGridMiddleware to satisfy analyzer - Refactor foreach to use Select() in test as suggested 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 100 out of 100 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (5)
src/AzureEventGridSimulator/Infrastructure/Middleware/EventGridMiddleware.cs:1
- The code performs a case-insensitive check using
StringComparison.OrdinalIgnoreCase, but then usesString.Replacewith the same comparison. Consider usingStringComparison.Ordinalfor theStartsWithcheck since the constant should match exactly, or document why case-insensitive comparison is needed for authorization headers.
src/AzureEventGridSimulator/Infrastructure/Settings/Subscribers/SubscribersSettings.cs:1 - The properties are now nullable with
setaccessors, but previously had default empty array initializers. The computed properties likeAllhandle null correctly, but this creates inconsistency. Consider usinginitinstead ofsetfor consistency with other settings classes, or restore the empty array defaults to maintain backward compatibility with code that may not expect null values.
src/AzureEventGridSimulator/Infrastructure/Settings/Subscribers/SubscribersSettings.cs:1 - The properties are now nullable with
setaccessors, but previously had default empty array initializers. The computed properties likeAllhandle null correctly, but this creates inconsistency. Consider usinginitinstead ofsetfor consistency with other settings classes, or restore the empty array defaults to maintain backward compatibility with code that may not expect null values.
src/AzureEventGridSimulator/Infrastructure/Settings/Subscribers/SubscribersSettings.cs:1 - The properties are now nullable with
setaccessors, but previously had default empty array initializers. The computed properties likeAllhandle null correctly, but this creates inconsistency. Consider usinginitinstead ofsetfor consistency with other settings classes, or restore the empty array defaults to maintain backward compatibility with code that may not expect null values.
src/AzureEventGridSimulator/Infrastructure/Settings/Subscribers/SubscribersSettings.cs:1 - The properties are now nullable with
setaccessors, but previously had default empty array initializers. The computed properties likeAllhandle null correctly, but this creates inconsistency. Consider usinginitinstead ofsetfor consistency with other settings classes, or restore the empty array defaults to maintain backward compatibility with code that may not expect null values.
|
|
||
| [Fact] | ||
| public void GivenDefaultSettings_ThenAllArraysAreEmpty() | ||
| public void GivenDefaultSettings_ThenAllArraysAreNull() |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test name 'GivenDefaultSettings_ThenAllArraysAreNull' uses 'Arrays' but these are properties that hold array references. Consider renaming to 'GivenDefaultSettings_ThenAllSubscriberPropertiesAreNull' for clarity.
| public void GivenDefaultSettings_ThenAllArraysAreNull() | |
| public void GivenDefaultSettings_ThenAllSubscriberPropertiesAreNull() |
| settings.Http.ShouldBeNull(); | ||
| settings.ServiceBus.ShouldBeNull(); | ||
| settings.StorageQueue.ShouldBeNull(); | ||
| settings.EventHub.ShouldBeNull(); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test name 'GivenDefaultSettings_ThenAllArraysAreNull' uses 'Arrays' but these are properties that hold array references. Consider renaming to 'GivenDefaultSettings_ThenAllSubscriberPropertiesAreNull' for clarity.
No description provided.