refactor!: Polyfill: Add universal ThrowIf* support for all .NET #470
Conversation
Refactor to provide polyfills for modern argument validation APIs (ArgumentNullException.ThrowIfNull, ArgumentException.ThrowIfNullOrEmpty/WhiteSpace, ArgumentOutOfRangeException.ThrowIf*) across all supported .NET targets, including .NET Standard 2.0 and .NET Framework. Mark legacy Argument.ThrowIf* methods as obsolete and delegate to new polyfills. Expand target frameworks, update CI for matrix builds, and add comprehensive unit tests. Remove legacy helpers and update package metadata to reflect universal polyfill focus.
samtrion
added
state:ready for merge
WalkthroughMoves argument validation helpers into three exception-specific polyfill classes, adds many cross-target attribute and IEnumerable polyfills, marks legacy Argument helpers obsolete and delegates to new APIs, updates CI and project target-variable configuration, and adds comprehensive unit tests for the new polyfills. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #470 +/- ##
===========================================
+ Coverage 96.29% 100.00% +3.70%
===========================================
Files 11 12 +1
Lines 27 92 +65
Branches 2 22 +20
===========================================
+ Hits 26 92 +66
+ Misses 1 0 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
.github/workflows/cicd.yml (1)
32-32: Consider documenting the rationale forwindows-latestas the build runner.While
windows-latestworks fine, usingubuntu-latestis typically faster and more cost-effective for cross-platform .NET builds. If there's a specific requirement for building on Windows (e.g., certain tooling or SDK features), consider documenting this choice in a comment for future maintainers.src/NetEvolve.Arguments/Argument_ThrowIfNotEqual.cs (1)
4-4: Remove unused using directive.The
using System.Collections.Generic;directive is not used in this file and should be removed.🔎 Proposed fix
namespace NetEvolve.Arguments; using System; -using System.Collections.Generic; using System.Diagnostics; using System.Runtime.CompilerServices;src/NetEvolve.Arguments/Argument_ThrowIfNullOrEmpty.cs (1)
35-38: Nullability inconsistency in parameter declaration.The parameter
IEnumerable<T> argumentis declared non-nullable, but it has[NotNull]attribute (used for nullable parameters). The polyfill inArgumentExceptionPolyfill.cs(line 71) declaresIEnumerable<T>? argument. Consider making this parameter nullable to match:🔎 Suggested fix
public static void ThrowIfNullOrEmpty<T>( - [NotNull] IEnumerable<T> argument, + [NotNull] IEnumerable<T>? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null ) => ArgumentException.ThrowIfNullOrEmpty(argument, paramName);tests/NetEvolve.Arguments.Tests.Unit/ArgumentNullExceptionPolyfillsTests.cs (1)
78-96: Potential test redundancy to review.The test at lines 78-86 (
ThrowIfNull_ReferenceType_WhenArgumentIsNotNull_DoesNotThrow) appears to duplicate the test at lines 20-28 (ThrowIfNull_Object_WhenArgumentIsNotNull_DoesNotThrow), as both testnew object().Additionally, the test at lines 88-96 (
ThrowIfNull_ValueType_WhenArgumentHasValue_DoesNotThrow) uses(object)123which is a boxed value type. Since the argument is still of typeobject, this doesn't meaningfully test a different code path from the other reference type tests.Consider consolidating or clarifying the intent of these test cases.
src/NetEvolve.Arguments/ArgumentOutOfRangeExceptionPolyfills.cs (1)
149-155: Inconsistent error message format.The error message at line 154 includes the parameter name in the message text itself:
$"{name} ('{value}') must be a non-negative and non-zero value."However, the parameter name is already passed as the first argument to the
ArgumentOutOfRangeExceptionconstructor. This is inconsistent with the other error messages in this file (lines 45, 97, 174, etc.) which don't duplicate the parameter name in the message text.🔎 Proposed fix for consistent error message format
[DoesNotReturn] private static void ThrowNegativeOrZero<T>(T value, string? name) => throw new ArgumentOutOfRangeException( name, value, - $"{name} ('{value}') must be a non-negative and non-zero value." + "Value must be non-negative and non-zero." );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (34)
.editorconfig.github/workflows/cicd.ymlDirectory.Build.propsREADME.mdsrc/NetEvolve.Arguments/Argument.cssrc/NetEvolve.Arguments/ArgumentExceptionPolyfill.cssrc/NetEvolve.Arguments/ArgumentNullExceptionPolyfills.cssrc/NetEvolve.Arguments/ArgumentOutOfRangeExceptionPolyfills.cssrc/NetEvolve.Arguments/Argument_EMPTY.cssrc/NetEvolve.Arguments/Argument_ThrowArgumentException.cssrc/NetEvolve.Arguments/Argument_ThrowArgumentNullException.cssrc/NetEvolve.Arguments/Argument_ThrowArgumentOutOfRangeException.cssrc/NetEvolve.Arguments/Argument_ThrowIfEqual.cssrc/NetEvolve.Arguments/Argument_ThrowIfGreaterThan.cssrc/NetEvolve.Arguments/Argument_ThrowIfGreaterThanOrEqual.cssrc/NetEvolve.Arguments/Argument_ThrowIfLessThan.cssrc/NetEvolve.Arguments/Argument_ThrowIfLessThanOrEqual.cssrc/NetEvolve.Arguments/Argument_ThrowIfNotEqual.cssrc/NetEvolve.Arguments/Argument_ThrowIfNull.cssrc/NetEvolve.Arguments/Argument_ThrowIfNullOrEmpty.cssrc/NetEvolve.Arguments/Argument_ThrowIfNullOrWhiteSpace.cssrc/NetEvolve.Arguments/NetEvolve.Arguments.csprojsrc/NetEvolve.Arguments/Polyfills/CallerArgumentExpressionAttribute.cssrc/NetEvolve.Arguments/Polyfills/DoesNotReturnAttribute.cssrc/NetEvolve.Arguments/Polyfills/DoesNotReturnIfAttribute.cssrc/NetEvolve.Arguments/Polyfills/IEnumerable_TryGetNonEnumeratedCount.cssrc/NetEvolve.Arguments/Polyfills/ModuleInitializerAttribute.cssrc/NetEvolve.Arguments/Polyfills/NotNullAttribute.cssrc/NetEvolve.Arguments/Polyfills/StackTraceHiddenAttribute.cstests/NetEvolve.Arguments.Tests.Unit/ArgumentExceptionPolyfillsTests.cstests/NetEvolve.Arguments.Tests.Unit/ArgumentNullExceptionPolyfillsTests.cstests/NetEvolve.Arguments.Tests.Unit/ArgumentOutOfRangeExceptionPolyfillsTests.cstests/NetEvolve.Arguments.Tests.Unit/ArgumentTests.cstests/NetEvolve.Arguments.Tests.Unit/NetEvolve.Arguments.Tests.Unit.csproj
💤 Files with no reviewable changes (5)
- src/NetEvolve.Arguments/Argument_ThrowArgumentException.cs
- src/NetEvolve.Arguments/Argument_EMPTY.cs
- src/NetEvolve.Arguments/Argument_ThrowArgumentOutOfRangeException.cs
- src/NetEvolve.Arguments/Argument_ThrowArgumentNullException.cs
- tests/NetEvolve.Arguments.Tests.Unit/ArgumentTests.cs
🧰 Additional context used
🧬 Code graph analysis (13)
src/NetEvolve.Arguments/Polyfills/ModuleInitializerAttribute.cs (3)
src/NetEvolve.Arguments/Polyfills/CallerArgumentExpressionAttribute.cs (1)
ExcludeFromCodeCoverage(14-28)src/NetEvolve.Arguments/Polyfills/DoesNotReturnIfAttribute.cs (1)
ExcludeFromCodeCoverage(12-29)src/NetEvolve.Arguments/Polyfills/IEnumerable_TryGetNonEnumeratedCount.cs (1)
ExcludeFromCodeCoverage(13-38)
src/NetEvolve.Arguments/Argument_ThrowIfLessThan.cs (8)
src/NetEvolve.Arguments/Argument_ThrowIfEqual.cs (1)
Obsolete(13-22)src/NetEvolve.Arguments/Argument_ThrowIfGreaterThan.cs (1)
Obsolete(13-22)src/NetEvolve.Arguments/Argument_ThrowIfGreaterThanOrEqual.cs (1)
Obsolete(13-22)src/NetEvolve.Arguments/Argument_ThrowIfLessThanOrEqual.cs (1)
Obsolete(13-22)src/NetEvolve.Arguments/Argument_ThrowIfNotEqual.cs (1)
Obsolete(14-23)src/NetEvolve.Arguments/Argument_ThrowIfNull.cs (2)
Obsolete(13-20)Obsolete(25-34)src/NetEvolve.Arguments/Argument_ThrowIfNullOrEmpty.cs (2)
Obsolete(17-24)Obsolete(31-38)src/NetEvolve.Arguments/Argument_ThrowIfNullOrWhiteSpace.cs (1)
Obsolete(15-22)
src/NetEvolve.Arguments/Polyfills/DoesNotReturnAttribute.cs (3)
src/NetEvolve.Arguments/Polyfills/CallerArgumentExpressionAttribute.cs (1)
ExcludeFromCodeCoverage(14-28)src/NetEvolve.Arguments/Polyfills/DoesNotReturnIfAttribute.cs (1)
ExcludeFromCodeCoverage(12-29)src/NetEvolve.Arguments/Polyfills/IEnumerable_TryGetNonEnumeratedCount.cs (1)
ExcludeFromCodeCoverage(13-38)
src/NetEvolve.Arguments/Argument_ThrowIfGreaterThan.cs (6)
src/NetEvolve.Arguments/Argument_ThrowIfEqual.cs (1)
Obsolete(13-22)src/NetEvolve.Arguments/Argument_ThrowIfGreaterThanOrEqual.cs (1)
Obsolete(13-22)src/NetEvolve.Arguments/Argument_ThrowIfLessThan.cs (1)
Obsolete(13-22)src/NetEvolve.Arguments/Argument_ThrowIfLessThanOrEqual.cs (1)
Obsolete(13-22)src/NetEvolve.Arguments/Argument_ThrowIfNotEqual.cs (1)
Obsolete(14-23)src/NetEvolve.Arguments/Argument_ThrowIfNullOrWhiteSpace.cs (1)
Obsolete(15-22)
tests/NetEvolve.Arguments.Tests.Unit/ArgumentExceptionPolyfillsTests.cs (2)
tests/NetEvolve.Arguments.Tests.Unit/ArgumentNullExceptionPolyfillsTests.cs (8)
Test(7-18)Test(20-28)Test(30-41)Test(43-52)Test(54-65)Test(67-76)Test(78-86)Test(88-96)tests/NetEvolve.Arguments.Tests.Unit/ArgumentOutOfRangeExceptionPolyfillsTests.cs (8)
Test(7-18)Test(20-30)Test(32-43)Test(45-53)Test(55-66)Test(68-77)Test(79-90)Test(92-100)
src/NetEvolve.Arguments/ArgumentExceptionPolyfill.cs (2)
src/NetEvolve.Arguments/ArgumentNullExceptionPolyfills.cs (1)
SuppressMessage(12-52)src/NetEvolve.Arguments/Polyfills/IEnumerable_TryGetNonEnumeratedCount.cs (1)
TryGetNonEnumeratedCount(21-37)
src/NetEvolve.Arguments/Polyfills/DoesNotReturnIfAttribute.cs (2)
src/NetEvolve.Arguments/Polyfills/CallerArgumentExpressionAttribute.cs (1)
ExcludeFromCodeCoverage(14-28)src/NetEvolve.Arguments/Polyfills/IEnumerable_TryGetNonEnumeratedCount.cs (1)
ExcludeFromCodeCoverage(13-38)
src/NetEvolve.Arguments/Polyfills/StackTraceHiddenAttribute.cs (3)
src/NetEvolve.Arguments/Polyfills/CallerArgumentExpressionAttribute.cs (1)
ExcludeFromCodeCoverage(14-28)src/NetEvolve.Arguments/Polyfills/DoesNotReturnIfAttribute.cs (1)
ExcludeFromCodeCoverage(12-29)src/NetEvolve.Arguments/Polyfills/IEnumerable_TryGetNonEnumeratedCount.cs (1)
ExcludeFromCodeCoverage(13-38)
src/NetEvolve.Arguments/Argument_ThrowIfNullOrEmpty.cs (8)
src/NetEvolve.Arguments/Argument_ThrowIfEqual.cs (1)
Obsolete(13-22)src/NetEvolve.Arguments/Argument_ThrowIfGreaterThan.cs (1)
Obsolete(13-22)src/NetEvolve.Arguments/Argument_ThrowIfGreaterThanOrEqual.cs (1)
Obsolete(13-22)src/NetEvolve.Arguments/Argument_ThrowIfLessThan.cs (1)
Obsolete(13-22)src/NetEvolve.Arguments/Argument_ThrowIfLessThanOrEqual.cs (1)
Obsolete(13-22)src/NetEvolve.Arguments/Argument_ThrowIfNotEqual.cs (1)
Obsolete(14-23)src/NetEvolve.Arguments/Argument_ThrowIfNull.cs (2)
Obsolete(13-20)Obsolete(25-34)src/NetEvolve.Arguments/Argument_ThrowIfNullOrWhiteSpace.cs (1)
Obsolete(15-22)
src/NetEvolve.Arguments/ArgumentNullExceptionPolyfills.cs (2)
src/NetEvolve.Arguments/ArgumentExceptionPolyfill.cs (1)
SuppressMessage(10-86)src/NetEvolve.Arguments/ArgumentOutOfRangeExceptionPolyfills.cs (1)
SuppressMessage(9-297)
src/NetEvolve.Arguments/ArgumentOutOfRangeExceptionPolyfills.cs (2)
src/NetEvolve.Arguments/ArgumentExceptionPolyfill.cs (1)
SuppressMessage(10-86)src/NetEvolve.Arguments/ArgumentNullExceptionPolyfills.cs (1)
SuppressMessage(12-52)
src/NetEvolve.Arguments/Argument.cs (9)
src/NetEvolve.Arguments/Argument_ThrowIfEqual.cs (1)
Argument(7-23)src/NetEvolve.Arguments/Argument_ThrowIfGreaterThan.cs (1)
Argument(7-23)src/NetEvolve.Arguments/Argument_ThrowIfGreaterThanOrEqual.cs (1)
Argument(7-23)src/NetEvolve.Arguments/Argument_ThrowIfLessThan.cs (1)
Argument(7-23)src/NetEvolve.Arguments/Argument_ThrowIfLessThanOrEqual.cs (1)
Argument(7-23)src/NetEvolve.Arguments/Argument_ThrowIfNotEqual.cs (1)
Argument(8-24)src/NetEvolve.Arguments/Argument_ThrowIfNull.cs (1)
Argument(8-35)src/NetEvolve.Arguments/Argument_ThrowIfNullOrEmpty.cs (1)
Argument(10-39)src/NetEvolve.Arguments/Argument_ThrowIfNullOrWhiteSpace.cs (1)
Argument(8-23)
src/NetEvolve.Arguments/Argument_ThrowIfNull.cs (2)
src/NetEvolve.Arguments/Argument_ThrowIfNullOrEmpty.cs (2)
Obsolete(17-24)Obsolete(31-38)src/NetEvolve.Arguments/Argument_ThrowIfNullOrWhiteSpace.cs (1)
Obsolete(15-22)
🔇 Additional comments (30)
.editorconfig (1)
45-47: LGTM!The increased line length for project files accommodates verbose XML elements like package references and long property values, which aligns with the expanded multi-targeting in this PR.
src/NetEvolve.Arguments/Polyfills/CallerArgumentExpressionAttribute.cs (1)
1-34: LGTM!The polyfill correctly guards with
!NETCOREAPP3_0_OR_GREATERmatching whenCallerArgumentExpressionAttributewas introduced. TheTypeForwardedTopattern for newer frameworks prevents type conflicts. The attributes (ExcludeFromCodeCoverage,DebuggerNonUserCode) appropriately mark this as infrastructure code.src/NetEvolve.Arguments/Polyfills/ModuleInitializerAttribute.cs (1)
1-25: LGTM!The polyfill correctly targets
!NET5_0_OR_GREATERwhich matches whenModuleInitializerAttributewas introduced. The implementation follows the same pattern as the other polyfills in this PR.src/NetEvolve.Arguments/Polyfills/NotNullAttribute.cs (1)
1-22: LGTM!The dual guard
!NETCOREAPP3_0_OR_GREATER && !NETSTANDARD2_1_OR_GREATERcorrectly reflects thatNotNullAttributeis available in both .NET Core 3.0+ and .NET Standard 2.1+. TheAttributeUsagetargets match the framework definition.src/NetEvolve.Arguments/Polyfills/IEnumerable_TryGetNonEnumeratedCount.cs (1)
21-37: LGTM!The polyfill correctly implements the core functionality of
TryGetNonEnumeratedCountfor .NET < 6.0. The check order (genericICollection<T>before non-genericICollection) is appropriate for type safety. The simplified implementation omits internal LINQ provider checks but covers the primary use cases (arrays, lists, and standard collections).src/NetEvolve.Arguments/Polyfills/DoesNotReturnAttribute.cs (1)
1-19: LGTM!The polyfill correctly implements
DoesNotReturnAttributewith the appropriate conditional guard for frameworks lacking the attribute. The implementation is consistent with the other polyfills in this PR.src/NetEvolve.Arguments/Polyfills/StackTraceHiddenAttribute.cs (1)
1-22: LGTM!The polyfill correctly targets
!NET6_0_OR_GREATERmatching whenStackTraceHiddenAttributewas introduced. The namespace (System.Diagnostics) andAttributeUsagetargets match the framework definition.tests/NetEvolve.Arguments.Tests.Unit/NetEvolve.Arguments.Tests.Unit.csproj (2)
17-19: LGTM!The
AssemblyAttributeforNetEvolve.Extensions.TUnit.UnitTestAttributeenables proper test discovery with the TUnit framework.
3-5: LGTM!Using
$(_TestTargetFrameworks)centralizes multi-targeting configuration across all test projects, and suppressingCS0618is appropriate since this PR marks legacyArgument.ThrowIf*methods as obsolete while tests still need to verify their behavior.src/NetEvolve.Arguments/NetEvolve.Arguments.csproj (1)
3-4: LGTM! Project configuration aligns with polyfill strategy.The use of
$(_ProjectTargetFrameworks)enables consistent multi-targeting across the solution, and the updated description accurately reflects the universal polyfill approach for ThrowIf* helpers.src/NetEvolve.Arguments/Argument_ThrowIfNullOrWhiteSpace.cs (1)
15-22: LGTM! Deprecation and delegation pattern is correct.The method is properly marked as obsolete with a clear migration path, and delegates to the framework/polyfill implementation of
ArgumentException.ThrowIfNullOrWhiteSpace.src/NetEvolve.Arguments/Argument_ThrowIfEqual.cs (1)
13-22: LGTM! Deprecation and delegation pattern is correct.The obsolete method correctly delegates to
ArgumentOutOfRangeException.ThrowIfEqualwhile preserving the generic constraintwhere T : IEquatable<T>.src/NetEvolve.Arguments/Argument_ThrowIfNotEqual.cs (1)
14-23: LGTM! Deprecation and delegation pattern is correct.The method is properly marked as obsolete and delegates to
ArgumentOutOfRangeException.ThrowIfNotEqualwhile preserving the generic constraint.src/NetEvolve.Arguments/Argument_ThrowIfGreaterThanOrEqual.cs (1)
13-22: LGTM! Deprecation and delegation pattern is correct.The obsolete method correctly delegates to
ArgumentOutOfRangeException.ThrowIfGreaterThanOrEqualwhile preserving the generic constraintwhere T : IComparable<T>.Directory.Build.props (1)
3-13: LGTM! Multi-targeting configuration is well-structured.The property definitions cleanly organize target frameworks into logical groups and conditionally include .NET Framework targets on Windows. The
CheckEolTargetFramework=falsesetting is appropriate for a polyfill library supporting multiple framework versions.Note: The configuration includes
net10.0as a target. Please ensure your build environment has an SDK version that recognizes this target framework moniker, as .NET 10 may still be in preview.src/NetEvolve.Arguments/Argument_ThrowIfLessThanOrEqual.cs (1)
13-22: LGTM! Deprecation and delegation pattern is correct.The obsolete method correctly delegates to
ArgumentOutOfRangeException.ThrowIfLessThanOrEqualwhile preserving the generic constraintwhere T : IComparable<T>.src/NetEvolve.Arguments/Argument.cs (1)
9-9: No concerns. The empty partial class declaration syntax is valid across all target frameworks and does not require C# 10+.src/NetEvolve.Arguments/Polyfills/DoesNotReturnIfAttribute.cs (1)
1-34: LGTM!The polyfill implementation follows the established pattern in the codebase (e.g.,
CallerArgumentExpressionAttribute). The conditional compilation correctly provides a local implementation for older frameworks and type forwards to the framework type for newer ones.tests/NetEvolve.Arguments.Tests.Unit/ArgumentOutOfRangeExceptionPolyfillsTests.cs (1)
1-6: Good test coverage for the polyfills.The test class provides comprehensive coverage for all
ArgumentOutOfRangeException.ThrowIf*polyfill methods across multiple numeric types (int,double,nint) and strings. The parameterized tests with[Arguments]effectively cover both throwing and non-throwing scenarios.src/NetEvolve.Arguments/Argument_ThrowIfLessThan.cs (1)
13-22: LGTM!The deprecation pattern is consistent with other
Argument.ThrowIf*methods in the codebase, correctly marking the method as obsolete and delegating to the polyfill implementation.src/NetEvolve.Arguments/Argument_ThrowIfNull.cs (2)
13-20: LGTM!The object overload is correctly marked obsolete and delegates to
ArgumentNullException.ThrowIfNull. The pattern is consistent with other deprecated methods.
25-34: LGTM!The unsafe pointer overload follows the same deprecation pattern. Note that
MethodImpl(AggressiveInlining)is appropriate here since the method now simply delegates to the polyfill.src/NetEvolve.Arguments/Argument_ThrowIfGreaterThan.cs (1)
13-22: LGTM!The deprecation pattern is consistent with the other
Argument.ThrowIf*methods, correctly delegating to the polyfill implementation.src/NetEvolve.Arguments/ArgumentNullExceptionPolyfills.cs (1)
1-34: Well-structured polyfill implementation.The object overload is correctly guarded with
#if !NET6_0_OR_GREATERsinceArgumentNullException.ThrowIfNull(object?, string?)was introduced in .NET 6. The extension syntax and namespace placement enable the familiarArgumentNullException.ThrowIfNull(...)call pattern on older frameworks.src/NetEvolve.Arguments/ArgumentExceptionPolyfill.cs (1)
15-61: LGTM!The string validation methods (
ThrowIfNullOrEmpty,ThrowIfNullOrWhiteSpace) are correctly guarded for frameworks prior to .NET 8 and properly handle null, empty, and whitespace cases with appropriate exception types.src/NetEvolve.Arguments/Argument_ThrowIfNullOrEmpty.cs (1)
17-24: LGTM!The string overload is correctly marked obsolete and delegates to the polyfill implementation, following the consistent pattern across all deprecated
Argument.ThrowIf*methods.tests/NetEvolve.Arguments.Tests.Unit/ArgumentExceptionPolyfillsTests.cs (1)
6-154: Comprehensive test coverage for ArgumentException polyfills!The test suite thoroughly covers all scenarios for the
ThrowIfNullOrEmptyandThrowIfNullOrWhiteSpacepolyfills:
- Null argument validation (expecting ArgumentNullException)
- Empty/whitespace validation (expecting ArgumentException)
- Valid input scenarios (no exception)
- Both string and generic enumerable overloads
The tests follow consistent patterns with the other polyfill test files and use appropriate assertion strategies.
README.md (1)
12-290: Excellent documentation for the polyfill architecture!The README comprehensively documents:
- The three-class polyfill architecture (ArgumentNullExceptionPolyfills, ArgumentExceptionPolyfills, ArgumentOutOfRangeExceptionPolyfills)
- Clear usage examples demonstrating the new API patterns
- Complete method documentation with native API references
- Framework compatibility table with appropriate notes about delegation behavior
- Proper attribution of Windows-only support for .NET Framework targets
The documentation provides clear guidance for users migrating to or using the polyfills across multiple .NET versions.
src/NetEvolve.Arguments/ArgumentOutOfRangeExceptionPolyfills.cs (2)
14-295: Well-structured polyfill implementation with proper conditional compilation.The implementation correctly:
- Uses conditional compilation to provide polyfills only for frameworks lacking native implementations (
!NET8_0_OR_GREATER, etc.)- Leverages
INumberBase<T>for .NET 7+ where available, falling back toIComparable<T>andIEquatable<T>constraints for older frameworks- Provides specialized overloads for
ninton pre-.NET 7 frameworks- Uses
[DoesNotReturn]attributes on helper methods- Includes proper XML documentation with links to official .NET API references
- Implements consistent exception message patterns (with one minor inconsistency noted separately)
The polyfills will seamlessly delegate to native implementations on modern frameworks while providing backward compatibility for older targets.
12-12: Theextension(...)syntax is the standard Polyfill library pattern and requires no changes.This project uses the Polyfill NuGet package (v9.5.0) for source generation. The
extension(ArgumentOutOfRangeException)syntax is the documented way to define polyfill extension methods; the source generator processes this syntax at compile time to create extension methods for older framework versions. The build system correctly handles this, and it generates the intended extension method implementations as designed.
tests/NetEvolve.Arguments.Tests.Unit/ArgumentOutOfRangeExceptionPolyfillsTests.cs
Show resolved
Hide resolved
tests/NetEvolve.Arguments.Tests.Unit/ArgumentOutOfRangeExceptionPolyfillsTests.cs
Show resolved
Hide resolved
|
@coderabbitai generate docstrings |
|
Warning Docstrings generation is disabled for your repository or organization. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 23497 @samtrion: Great news! Your polyfill classes already have comprehensive XML documentation. I've reviewed all three files and found they're well-documented with summaries, parameter descriptions, exception documentation, and links to Microsoft documentation. However, I noticed a few minor improvements that can be made: 1. Missing seealso link in ArgumentExceptionPolyfill.csThe /// <summary>Throws an exception if <paramref name="argument"/> is <see langword="null"/> or empty.</summary>
/// <typeparam name="T">The type of objects to enumerate.</typeparam>
/// <param name="argument">The enumerable argument to validate as non-<see langword="null"/> and non-empty.</param>
/// <param name="paramName">The name of the parameter with which <paramref name="argument"/> corresponds.</param>
/// <exception cref="ArgumentNullException"><paramref name="argument"/> is <see langword="null"/>.</exception>
/// <exception cref="ArgumentException"><paramref name="argument"/> is empty.</exception>
/// <remarks>The <paramref name="paramName"/> parameter is included to support the <see cref="CallerArgumentExpressionAttribute"/> attribute. It's recommended that you don't pass a value for this parameter and let the name of <paramref name="argument"/> be used instead.</remarks>
public static void ThrowIfNullOrEmpty<T>(
[NotNull] IEnumerable<T>? argument,
[CallerArgumentExpression(nameof(argument))] string? paramName = null
)2. Documentation for private helper methods in ArgumentOutOfRangeExceptionPolyfills.csWhile optional, it's good practice to document private methods too: /// <summary>Throws an <see cref="ArgumentOutOfRangeException"/> with a zero-value message.</summary>
/// <param name="paramName">The name of the parameter that caused the exception.</param>
/// <exception cref="ArgumentOutOfRangeException">Always thrown.</exception>
[DoesNotReturn]
private static void ThrowZero(string? paramName) =>
throw new ArgumentOutOfRangeException(paramName, "Value must not be zero.");
/// <summary>Throws an <see cref="ArgumentOutOfRangeException"/> with a negative-value message.</summary>
/// <typeparam name="T">The type of the value.</typeparam>
/// <param name="value">The actual value of the argument.</param>
/// <param name="paramName">The name of the parameter that caused the exception.</param>
/// <exception cref="ArgumentOutOfRangeException">Always thrown.</exception>
[DoesNotReturn]
private static void ThrowNegative<T>(T value, string? paramName) =>
throw new ArgumentOutOfRangeException(paramName, value, "Value must be non-negative.");
/// <summary>Throws an <see cref="ArgumentOutOfRangeException"/> with a negative-or-zero-value message.</summary>
/// <typeparam name="T">The type of the value.</typeparam>
/// <param name="value">The actual value of the argument.</param>
/// <param name="name">The name of the parameter that caused the exception.</param>
/// <exception cref="ArgumentOutOfRangeException">Always thrown.</exception>
[DoesNotReturn]
private static void ThrowNegativeOrZero<T>(T value, string? name) =>
throw new ArgumentOutOfRangeException(
name,
value,
$"{name} ('{value}') must be a non-negative and non-zero value."
);Overall, your documentation is already excellent and follows best practices with clear descriptions, proper XML tags, and helpful links to Microsoft's documentation. The above changes are minor polish that would make it even more complete. Would you like me to provide the complete updated files with these improvements? |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/NetEvolve.Arguments/NetEvolve.Arguments.csproj (1)
4-4: Consider shortening the package description for better readability.While the description is comprehensive and accurate, at 341 characters it may be verbose for package metadata. Consider a more concise version that highlights the key value proposition.
💡 Example concise description
- <Description>A universal polyfill library that provides modern ArgumentNullException.ThrowIf* and ArgumentException.ThrowIf* helper methods across all .NET runtimes (.NET Standard 2.0+, .NET Framework 4.6.2+, .NET 6.0+), enabling consistent argument validation patterns regardless of target framework version.</Description> + <Description>Universal polyfill library providing modern ArgumentNullException.ThrowIf* and ArgumentException.ThrowIf* helpers across all .NET runtimes (.NET Standard 2.0+, .NET Framework 4.6.2+, .NET 6+).</Description>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
src/NetEvolve.Arguments/NetEvolve.Arguments.csprojtests/NetEvolve.Arguments.Tests.Unit/NetEvolve.Arguments.Tests.Unit.csproj
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/NetEvolve.Arguments.Tests.Unit/NetEvolve.Arguments.Tests.Unit.csproj
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Tests / Run Tests / Testing .NET solution
- GitHub Check: Build & Tests / Code Formatting / Validate Code Formatting
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/NetEvolve.Arguments/ArgumentOutOfRangeExceptionPolyfills.cs (2)
174-174: Rename parameter to match other helpers.The parameter is named
namebut should beparamNamefor consistency with the other helper methods (ThrowZeroat line 54 andThrowNegative<T>at line 118).🔎 Proposed fix
- private static void ThrowNegativeOrZero<T>(T value, string? name) => + private static void ThrowNegativeOrZero<T>(T value, string? paramName) => throw new ArgumentOutOfRangeException( - name, + paramName, value, - $"{name} ('{value}') must be a non-negative and non-zero value." + $"{paramName} ('{value}') must be a non-negative and non-zero value." );
188-193: Consider standardizing the error message format.The error message includes the parameter name in the message string itself (
"{name} ('{value}') must be..."), which differs from all other error messages in this file. Other methods pass the parameter name as the first argument toArgumentOutOfRangeExceptionand keep it out of the message string (e.g., lines 212, 234, 259, 282, 307, 330).While including the parameter name might provide additional context, the inconsistency may be unintentional.
🔎 Proposed fix for consistency
[DoesNotReturn] private static void ThrowNegativeOrZero<T>(T value, string? paramName) => throw new ArgumentOutOfRangeException( paramName, value, - $"{paramName} ('{value}') must be a non-negative and non-zero value." + "Value must be non-negative and non-zero." );Note: The exception constructor already includes the parameter name and value in its formatted output, so including them in the message string may be redundant.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
.github/workflows/cicd.ymlsrc/NetEvolve.Arguments/ArgumentOutOfRangeExceptionPolyfills.cssrc/NetEvolve.Arguments/NetEvolve.Arguments.csprojsrc/NetEvolve.Arguments/Polyfills/IEnumerable_TryGetNonEnumeratedCount.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NetEvolve.Arguments/Polyfills/IEnumerable_TryGetNonEnumeratedCount.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/NetEvolve.Arguments/ArgumentOutOfRangeExceptionPolyfills.cs (2)
src/NetEvolve.Arguments/ArgumentNullExceptionPolyfills.cs (1)
SuppressMessage(12-52)src/NetEvolve.Arguments/ArgumentExceptionPolyfill.cs (1)
SuppressMessage(10-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Tests / Run Tests / Testing .NET solution
- GitHub Check: Build & Tests / Code Formatting / Validate Code Formatting
🔇 Additional comments (8)
.github/workflows/cicd.yml (1)
28-28: TherunsOnBuildparameter is fully supported by the workflow. No action required.src/NetEvolve.Arguments/NetEvolve.Arguments.csproj (2)
3-3: LGTM! Variable-based target framework management is a best practice.Using the centralized
$(_ProjectTargetFrameworks)variable improves maintainability and ensures consistent multi-targeting across the project.
9-9: LGTM! Typo fixed and tags appropriately expanded.The previous typo ("clausel" → "clause") has been corrected, and the package tags have been comprehensively expanded with relevant keywords (argument-validation, polyfill, null-check, parameter-validation, etc.) that will improve package discoverability.
src/NetEvolve.Arguments/ArgumentOutOfRangeExceptionPolyfills.cs (5)
1-13: LGTM! Proper polyfill structure.The file header, namespace declaration, and class structure follow the correct pattern for System namespace polyfills, consistent with the related ArgumentNullExceptionPolyfills and ArgumentExceptionPolyfills classes.
14-56: LGTM! Well-structured polyfill with appropriate fallbacks.The
ThrowIfZeroimplementation correctly usesINumberBase<T>for .NET 7+ and falls back toIEquatable<T>for older versions. The helper method is properly wrapped in the same conditional block and includes theDoesNotReturnattribute.
58-121: LGTM! Correct conditional compilation and nint handling.The
ThrowIfNegativeimplementation properly includes a separatenintoverload for pre-.NET 7 targets, asINumberBase<T>handlesnintnaturally on .NET 7+. The helper method is correctly positioned within the outer conditional block.
195-237: LGTM! Correct equality comparison logic.Both
ThrowIfEqualandThrowIfNotEqualcorrectly useEqualityComparer<T>.Default.Equalsfor robust equality comparison, with appropriateIEquatable<T>?constraints that support nullable types.
239-333: LGTM! All comparison methods implement correct logic.The four comparison methods (
ThrowIfGreaterThan,ThrowIfGreaterThanOrEqual,ThrowIfLessThan,ThrowIfLessThanOrEqual) all correctly useCompareTowith appropriate comparison operators, and their error messages accurately describe the validation constraints.
Refactor to provide polyfills for modern argument validation APIs (ArgumentNullException.ThrowIfNull, ArgumentException.ThrowIfNullOrEmpty/WhiteSpace, ArgumentOutOfRangeException.ThrowIf*) across all supported .NET targets, including .NET Standard 2.0 and .NET Framework. Mark legacy Argument.ThrowIf* methods as obsolete and delegate to new polyfills. Expand target frameworks, update CI for matrix builds, and add comprehensive unit tests. Remove legacy helpers and update package metadata to reflect universal polyfill focus.
Summary by CodeRabbit
New Features
Deprecated
Documentation
CI / Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.