Enable nullable in project but disable in files#115
Conversation
WalkthroughThe test project's project file enables nullable reference types globally by adding Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes The changes are homogeneous and repetitive—enabling nullable types at the project level and selectively disabling them in individual test files through straightforward preprocessor directives. No logic modifications, control flow changes, or public API alterations are present. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 0
🧹 Nitpick comments (9)
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyDomainHttpErrorAttribute.cs (1)
5-13: Consider refactoring to use file-scoped namespace and primary constructor.Per coding guidelines, this code could be modernized:
- Use file-scoped namespace (reduces nesting)
- Use primary constructor (more concise)
Apply this refactor:
-namespace Activout.RestClient.Newtonsoft.Json.Test.DomainExceptions -{ - internal class MyDomainHttpErrorAttribute : DomainHttpErrorAttribute - { - public MyDomainHttpErrorAttribute(HttpStatusCode httpStatusCode, MyDomainErrorEnum domainErrorValue) : base( - httpStatusCode, domainErrorValue) - { - } - } -} +namespace Activout.RestClient.Newtonsoft.Json.Test.DomainExceptions; + +internal class MyDomainHttpErrorAttribute(HttpStatusCode httpStatusCode, MyDomainErrorEnum domainErrorValue) + : DomainHttpErrorAttribute(httpStatusCode, domainErrorValue);Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyDomainErrorAttribute.cs (1)
4-11: Consider refactoring to use file-scoped namespace and primary constructor.Per coding guidelines, this code could be modernized with file-scoped namespace and primary constructor.
Apply this refactor:
-namespace Activout.RestClient.Newtonsoft.Json.Test.DomainExceptions -{ - internal class MyDomainErrorAttribute : DomainErrorAttribute - { - public MyDomainErrorAttribute(MyApiError apiValue, MyDomainErrorEnum domainValue) : base(apiValue, domainValue) - { - } - } -} +namespace Activout.RestClient.Newtonsoft.Json.Test.DomainExceptions; + +internal class MyDomainErrorAttribute(MyApiError apiValue, MyDomainErrorEnum domainValue) + : DomainErrorAttribute(apiValue, domainValue);Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyApiError.cs (1)
2-9: Consider using file-scoped namespace.Per coding guidelines, this enum definition could use file-scoped namespace to reduce nesting.
Apply this refactor:
-namespace Activout.RestClient.Newtonsoft.Json.Test.DomainExceptions -{ - public enum MyApiError - { - Foo = 4, - Bar = 5 - } -} +namespace Activout.RestClient.Newtonsoft.Json.Test.DomainExceptions; + +public enum MyApiError +{ + Foo = 4, + Bar = 5 +}Activout.RestClient.Newtonsoft.Json.Test/NewtonsoftJsonDeserializerTest.cs (1)
14-20: Consider using file-scoped namespace and record for DTO.Per coding guidelines:
- Use file-scoped namespace to reduce nesting
- Use records for DTOs and immutable data structures
The
Dataclass could be a record:-namespace Activout.RestClient.Newtonsoft.Json.Test -{ - [SuppressMessage("ReSharper", "ClassNeverInstantiated.Global")] - public class Data - { - public string Value { get; set; } - } +namespace Activout.RestClient.Newtonsoft.Json.Test; + +[SuppressMessage("ReSharper", "ClassNeverInstantiated.Global")] +public record Data(string Value);Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorObjectTests.cs (1)
13-23: Consider using file-scoped namespace and primary constructor.Per coding guidelines,
MyDomainErrorObjectcould be modernized with file-scoped namespace and primary constructor.Apply this refactor:
-namespace Activout.RestClient.Newtonsoft.Json.Test.DomainExceptions -{ - internal class MyDomainErrorObject - { - public MyDomainErrorEnum ErrorEnum { get; } - - public MyDomainErrorObject(MyDomainErrorEnum errorEnum) - { - ErrorEnum = errorEnum; - } - } +namespace Activout.RestClient.Newtonsoft.Json.Test.DomainExceptions; + +internal class MyDomainErrorObject(MyDomainErrorEnum errorEnum) +{ + public MyDomainErrorEnum ErrorEnum { get; } = errorEnum; +}Activout.RestClient.Newtonsoft.Json.Test/RestClientTests.cs (1)
17-17: Consider using file-scoped namespace.Per coding guidelines, the namespace declaration could be file-scoped to reduce nesting.
Change:
-namespace Activout.RestClient.Newtonsoft.Json.Test -{ +namespace Activout.RestClient.Newtonsoft.Json.Test;(and remove the closing brace at the end of the file)
Activout.RestClient.Newtonsoft.Json.Test/SimpleValueObjectTest.cs (1)
11-28: Consider using file-scoped namespace, primary constructor, and record for DTO.Per coding guidelines, this code could be modernized:
- Use file-scoped namespace
- Use primary constructor for
MySimpleValueObject- Use record for
Stuff(it's a DTO)Apply this refactor:
-namespace Activout.RestClient.Newtonsoft.Json.Test -{ - public class MySimpleValueObject - { - public string Value { get; } - - public MySimpleValueObject(string value) - { - Value = value; - } - } - - public class Stuff - { - public MySimpleValueObject FooBar { get; set; } - - public int? NullableInteger { get; set; } - } +namespace Activout.RestClient.Newtonsoft.Json.Test; + +public class MySimpleValueObject(string value) +{ + public string Value { get; } = value; +} + +public record Stuff +{ + public MySimpleValueObject? FooBar { get; set; } + public int? NullableInteger { get; set; } +}Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyApiErrorResponse.cs (1)
2-9: Consider using file-scoped namespace and record for DTO.Per coding guidelines, this DTO class could be modernized with file-scoped namespace and record syntax.
Apply this refactor:
-namespace Activout.RestClient.Newtonsoft.Json.Test.DomainExceptions -{ - public class MyApiErrorResponse - { - [MyDomainError(MyApiError.Foo, MyDomainErrorEnum.DomainFoo)] - public MyApiError Code { get; set; } - } -} +namespace Activout.RestClient.Newtonsoft.Json.Test.DomainExceptions; + +public record MyApiErrorResponse +{ + [MyDomainError(MyApiError.Foo, MyDomainErrorEnum.DomainFoo)] + public MyApiError Code { get; set; } +}Activout.RestClient.Newtonsoft.Json.Test/MovieReviews/IMovieReviewService.cs (1)
1-1: Intentional nullable disable for incremental adoption.The approach of enabling nullable project-wide while disabling it per-file is a valid incremental migration strategy. This prevents breaking existing test code while setting up the infrastructure for future nullable adoption.
Consider tracking this as technical debt. Future work could gradually enable nullable in test files and annotate reference type parameters (e.g.,
movieId,reviewId) to catch potential null references at compile time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
Activout.RestClient.Newtonsoft.Json.Test/Activout.RestClient.Newtonsoft.Json.Test.csproj(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DefaultDomainExceptionErrorObjectTests.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorEnumTests.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorObjectTests.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyApiError.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyApiErrorResponse.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyDomainErrorAttribute.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyDomainErrorEnum.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyDomainHttpErrorAttribute.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/LoggerFactoryHelpers.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/MovieReviews/ErrorResponse.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/MovieReviews/IMovieReviewService.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/MovieReviews/Movie.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/MovieReviews/Review.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/NewtonsoftJsonDeserializerTest.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/RestClientTests.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/SerializationOrderTest.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/SimpleValueObjectTest.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Use [] list syntax for collections
Use file-scoped namespaces
Use primary constructors wherever possible
Use records for data transfer objects (DTOs) and immutable data structures
Use var for local variable declarations when possible
Use expression-bodied members for simple methods and properties
Use pattern matching for type checks and deconstruction
Use using statements for resource management
Use async and await for asynchronous programming
Never include "Async" in method names
Only include comments that explain why something is done, not what is done
Files:
Activout.RestClient.Newtonsoft.Json.Test/RestClientTests.csActivout.RestClient.Newtonsoft.Json.Test/LoggerFactoryHelpers.csActivout.RestClient.Newtonsoft.Json.Test/SimpleValueObjectTest.csActivout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyDomainErrorAttribute.csActivout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyDomainHttpErrorAttribute.csActivout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyApiErrorResponse.csActivout.RestClient.Newtonsoft.Json.Test/NewtonsoftJsonDeserializerTest.csActivout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyApiError.csActivout.RestClient.Newtonsoft.Json.Test/SerializationOrderTest.csActivout.RestClient.Newtonsoft.Json.Test/MovieReviews/IMovieReviewService.csActivout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorEnumTests.csActivout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorObjectTests.csActivout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DefaultDomainExceptionErrorObjectTests.csActivout.RestClient.Newtonsoft.Json.Test/MovieReviews/ErrorResponse.csActivout.RestClient.Newtonsoft.Json.Test/MovieReviews/Movie.csActivout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyDomainErrorEnum.csActivout.RestClient.Newtonsoft.Json.Test/MovieReviews/Review.cs
**/{global.json,Directory.Build.props,Directory.Build.targets,*.csproj}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use features from the latest stable .NET (9.x) and C# (13)
Files:
Activout.RestClient.Newtonsoft.Json.Test/Activout.RestClient.Newtonsoft.Json.Test.csproj
🔇 Additional comments (18)
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyDomainHttpErrorAttribute.cs (1)
1-1: Nullable disable directive is appropriate for gradual migration.The
#nullable disabledirective is a valid approach for gradually migrating to nullable reference types. This allows the project to enable nullable warnings globally while addressing each file incrementally.Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyDomainErrorAttribute.cs (1)
1-1: Nullable disable directive is appropriate for gradual migration.The
#nullable disabledirective follows the same valid migration pattern as other files in this PR.Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyApiError.cs (1)
1-1: Nullable disable directive is appropriate for gradual migration.Consistent with the PR's migration strategy.
Activout.RestClient.Newtonsoft.Json.Test/NewtonsoftJsonDeserializerTest.cs (1)
1-1: Nullable disable directive is appropriate for gradual migration.This follows the PR's strategy of enabling nullable project-wide while incrementally migrating files.
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorObjectTests.cs (1)
1-1: Nullable disable directive is appropriate for gradual migration.Consistent with the PR's nullable reference type migration strategy.
Activout.RestClient.Newtonsoft.Json.Test/RestClientTests.cs (1)
1-1: Nullable disable directive is appropriate for gradual migration.The directive follows the PR's migration pattern consistently.
Activout.RestClient.Newtonsoft.Json.Test/SimpleValueObjectTest.cs (1)
1-1: Nullable disable directive is appropriate for gradual migration.Consistent with the PR's nullable migration approach.
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyApiErrorResponse.cs (1)
1-1: Nullable disable directive is appropriate for gradual migration.The directive aligns with the PR's migration strategy.
Activout.RestClient.Newtonsoft.Json.Test/Activout.RestClient.Newtonsoft.Json.Test.csproj (2)
6-6: Verify target framework against coding guidelines.The coding guidelines specify using features from the latest stable .NET (9.x), but this project targets
net8.0. Consider whether the project should be upgraded tonet9.0to align with the guidelines.As per coding guidelines.
8-8: Good incremental nullable migration strategy.Enabling nullable reference types at the project level while individual files opt out with
#nullable disableis a solid approach for gradual migration. This allows new code to benefit from nullable analysis while existing code can be migrated file-by-file.Note that the per-file
#nullable disabledirectives should be considered temporary and removed as files are updated to properly handle nullability.Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DefaultDomainExceptionErrorObjectTests.cs (1)
1-1: Consistent nullable opt-out for gradual migration.The
#nullable disabledirective appropriately opts this file out of nullable reference type checking as part of the project-wide nullable migration strategy. This should be removed once the file is updated to handle nullable reference types properly.Activout.RestClient.Newtonsoft.Json.Test/MovieReviews/Review.cs (1)
1-1: Consistent nullable opt-out for gradual migration.The
#nullable disabledirective appropriately opts this file out of nullable reference type checking as part of the project-wide nullable migration strategy. This should be removed once the file is updated to handle nullable reference types properly.Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/MyDomainErrorEnum.cs (1)
1-1: Consistent nullable opt-out for gradual migration.The
#nullable disabledirective appropriately opts this file out of nullable reference type checking as part of the project-wide nullable migration strategy. This should be removed once the file is updated to handle nullable reference types properly.Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorEnumTests.cs (1)
1-1: Consistent nullable opt-out for gradual migration.The
#nullable disabledirective appropriately opts this file out of nullable reference type checking as part of the project-wide nullable migration strategy. This should be removed once the file is updated to handle nullable reference types properly.Activout.RestClient.Newtonsoft.Json.Test/SerializationOrderTest.cs (1)
1-1: Consistent nullable opt-out for gradual migration.The
#nullable disabledirective appropriately opts this file out of nullable reference type checking as part of the project-wide nullable migration strategy. This should be removed once the file is updated to handle nullable reference types properly.Activout.RestClient.Newtonsoft.Json.Test/MovieReviews/Movie.cs (1)
1-1: Consistent nullable opt-out for gradual migration.The
#nullable disabledirective appropriately opts this file out of nullable reference type checking as part of the project-wide nullable migration strategy. This should be removed once the file is updated to handle nullable reference types properly.Activout.RestClient.Newtonsoft.Json.Test/MovieReviews/ErrorResponse.cs (1)
1-1: Consistent nullable opt-out for gradual migration.The
#nullable disabledirective appropriately opts this file out of nullable reference type checking as part of the project-wide nullable migration strategy. This should be removed once the file is updated to handle nullable reference types properly.Activout.RestClient.Newtonsoft.Json.Test/LoggerFactoryHelpers.cs (1)
1-1: Consistent with the nullable migration strategy.This follows the same incremental adoption pattern as other test files in this PR.
twogood
deleted the
nullable-enable-start-Activout.RestClient.Newtonsoft.Json.Test
branch
1 participant
Summary by CodeRabbit