-
-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add polyfill for ObjectDisposedException with ThrowIf methods #499
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
WalkthroughThis PR introduces a conditional polyfill for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/NetEvolve.Arguments.Tests.Unit/ObjectDisposedExceptionPolyfillsTests.cs (1)
71-90: Disposed flag tests demonstrate real-world usage pattern.These tests effectively show the intended usage of
ThrowIfwith anIsDisposedproperty.Note: In line 84,
using varcombined with an explicitDispose()call results in double-disposal. This is safe here sinceDisposableTestClass.Dispose()is idempotent, but you could alternatively remove theusingkeyword in this specific test since the explicitDispose()is the intent.Optional: Remove `using` to avoid double-dispose
[Test] public void ThrowIf_WithDisposedFlag_WhenDisposed_ThrowsObjectDisposedException() { - using var disposable = new DisposableTestClass(); + var disposable = new DisposableTestClass(); disposable.Dispose(); void Act() => ObjectDisposedException.ThrowIf(disposable.IsDisposed, disposable); _ = Assert.Throws<ObjectDisposedException>(Act); }
📜 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/ObjectDisposedExceptionPolyfills.cstests/NetEvolve.Arguments.Tests.Unit/ObjectDisposedExceptionPolyfillsTests.cs
🔇 Additional comments (9)
src/NetEvolve.Arguments/ObjectDisposedExceptionPolyfills.cs (4)
1-12: File setup and class declaration look correct.The preprocessor guard for
!NET7_0_OR_GREATERcorrectly limits this polyfill to older .NET versions. TheSystemnamespace placement is intentional to mirror the native API.One minor observation: the
CA1034suppression for "Nested types should not be visible" may be unnecessary since the C# 13extensionblock is not a traditional nested type. Consider verifying if this suppression is actually triggered by analyzers.
14-27: ThrowIf with object instance implementation is correct.The logic properly checks the condition and delegates to the helper method. The
[DoesNotReturnIf(true)]attribute correctly informs the compiler about the control flow.
29-40: ThrowIf with Type implementation is correct.Mirrors the object overload appropriately. The implementation matches the .NET 7+ API behavior.
42-55: Private helper methods handle null correctly.The null-conditional operators (
instance?.GetType().FullNameandtype?.FullName) ensure graceful handling when null is passed, matching the native .NET behavior whereObjectDisposedException.ObjectNamereturns an empty string for null object names.tests/NetEvolve.Arguments.Tests.Unit/ObjectDisposedExceptionPolyfillsTests.cs (5)
7-16: Test correctly validates exception throwing with object instance.The test properly verifies that
ObjectDisposedExceptionis thrown and thatObjectNamecontains the full type name.
18-26: Test validates non-throwing path.Correctly verifies that when condition is
false, no exception is thrown and the instance remains valid.
28-47: Type overload tests are comprehensive.Both true and false condition paths are properly tested for the
Typeparameter overload.
49-69: Null input tests correctly verify edge case behavior.These tests validate that passing
nullfor eitherinstanceortyperesults inObjectNamebeing an empty string, which matches the .NET runtime behavior forObjectDisposedExceptionconstructed with a null object name.
92-97: DisposableTestClass is a clean, minimal test helper.The implementation is appropriately simple for testing purposes. The
IsDisposedproperty correctly tracks disposal state.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #499 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 12 13 +1
Lines 92 100 +8
Branches 22 26 +4
=========================================
+ Hits 92 100 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.