-
-
Notifications
You must be signed in to change notification settings - Fork 0
Added Queryable Extensions #71
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
- Fixes #70 Initial round of helpers to streamline LINQ & EFCore Queries
WalkthroughAdds a new QueryableExtensions static class with five IQueryable helpers (WhereIf, OrderByIf, OrderByDescendingIf, GetPage, DistinctBy) and a corresponding test suite that exercises conditional filtering, conditional ordering, paging, and distinct projection across an in-memory dataset. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
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: 6
🧹 Nitpick comments (4)
src/NetCore.Utilities.Tests/QueryableExtensionsTests.cs (4)
29-46: Consider adding edge case tests.The current tests cover the happy path well, but consider adding tests for:
- Null parameter handling (once null validation is added to the implementation)
- Empty queryable sequences
- Multiple chained
WhereIfcallsExample test for empty queryable:
[Fact] public void WhereIf_EmptyQueryable_ReturnsEmpty() { var query = Enumerable.Empty<TestEntity>().AsQueryable(); var result = query.WhereIf(true, x => x.Name == "Alpha").ToList(); Assert.Empty(result); }
48-82: LGTM! Consider adding edge case tests for consistency.The ordering tests are well-structured and cover both conditional branches effectively. For consistency with other test methods, consider adding edge case tests (null parameters, empty queryables) once the implementation adds proper validation.
95-106: Test doesn't verify deterministic element selection.The test confirms that distinct names are returned but doesn't verify which specific element is selected when duplicates exist (e.g., which "Alpha" entity—Id 1 or Id 4—is returned). This masks the non-deterministic behavior of the
GroupBy().Select(x => x.First())implementation.Consider adding a test that verifies deterministic behavior:
[Fact] public void DistinctBy_ConsecutiveRuns_ReturnsSameElements() { var query = GetTestEntities(); var result1 = query.DistinctBy(x => x.Name).OrderBy(x => x.Name).ToList(); var result2 = query.DistinctBy(x => x.Name).OrderBy(x => x.Name).ToList(); // Verify same IDs are returned for duplicate names Assert.Equal(result1.Select(x => x.Id), result2.Select(x => x.Id)); }Note: This test may fail with the current implementation if the underlying query execution order changes.
1-107: Consider expanding test coverage for robustness.The test suite provides good coverage of happy path scenarios. To improve robustness, consider adding:
- Null parameter tests for all methods (once validation is added)
- Edge case tests for boundary conditions
- Tests with empty queryables
- Tests for method chaining scenarios (e.g.,
WhereIf().OrderByIf())Example method chaining test:
[Fact] public void MultipleExtensions_Chained_WorksCorrectly() { var query = GetTestEntities(); var result = query .WhereIf(true, x => x.Name != "Gamma") .OrderByIf(true, x => x.Name) .GetPage(1, 2) .ToList(); Assert.Equal(2, result.Count); Assert.Equal(new[] { "Alpha", "Alpha" }, result.Select(x => x.Name).ToArray()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NetCore.Utilities.Tests/QueryableExtensionsTests.cs(1 hunks)src/NetCore.Utilities/QueryableExtensions.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/NetCore.Utilities.Tests/QueryableExtensionsTests.cs (1)
src/NetCore.Utilities/QueryableExtensions.cs (5)
IQueryable(20-30)IQueryable(41-47)IQueryable(58-64)IQueryable(74-80)IQueryable(90-95)
src/NetCore.Utilities/QueryableExtensions.cs (1)
src/NetCore.Utilities.Tests/QueryableExtensionsTests.cs (1)
IQueryable(12-21)
⏰ 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). (1)
- GitHub Check: Analyze Code Quality
🔇 Additional comments (2)
src/NetCore.Utilities.Tests/QueryableExtensionsTests.cs (1)
12-27: LGTM!The test data setup is well-structured with appropriate test entities, including a duplicate name ("Alpha") that effectively tests the
DistinctByfunctionality.src/NetCore.Utilities/QueryableExtensions.cs (1)
90-95: Remove the suggestion to use built-in DistinctBy.This is a utility library that does not depend on EF Core, so it cannot assume the built-in
DistinctBymethod is available to consumers. The custom implementation usingGroupBy().Select(x => x.First())is appropriate as an extension method onIQueryable<TSource>that works across different LINQ providers and EF Core versions. The current implementation is correct and necessary for a cross-framework utility library.Likely an incorrect or invalid review comment.
|



Initial round of helpers to streamline LINQ & EFCore Queries
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.