Fix/Improve TruncatedCollectionOfT #1492
Conversation
|
|
||
| bool isTruncated = false; | ||
| int count = 0; | ||
| await foreach (var item in source.Take(checked(capacity + 1)).WithCancellation(cancellationToken).ConfigureAwait(false)) |
There was a problem hiding this comment.
I don't quite understand. Isn't it strange to enumerate an IAsyncENumerable to put it in a list and then put it back in a new IAsyncEnumerable?
Shouldn't we stream the data completely? From what I understood, the only real problem is that the count was accessed at the beginning, but this is no longer the case since #1393
Shouldn't we introduce a TrucatedEnumerable instead?
There was a problem hiding this comment.
Thanks @rpallares for pointing this out. I have updated it. Could you take another look.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors TruncatedCollection<T> to use composition instead of inheriting from List<T>, adds support for asynchronous truncation via IAsyncEnumerable<T>, and introduces corresponding static Create methods and tests.
- Refactored
TruncatedCollection<T>to implementIReadOnlyList<T>andIAsyncEnumerable<T>, with internal factory methods - Introduced
TruncatedAsyncEnumerable<T>andTruncationStateto support async enumeration and truncation tracking - Expanded unit tests to cover the new sync, queryable, and async creation methods
- Updated project file to include
System.Linq.AsyncEnumerablepackage for async LINQ extensions
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/.../TruncatedCollectionOfTTest.cs | Added tests for Create overloads (sync, queryable, async) |
| src/.../Query/Container/TruncatedCollectionOfT.cs | Refactored class to use composition, added async support |
| src/.../Query/Container/TruncatedAsyncEnumerableOfT.cs | New async enumerable type for truncation |
| src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.csproj | Added reference to System.Linq.AsyncEnumerable preview package |
Comments suppressed due to low confidence (2)
test/Microsoft.AspNetCore.OData.Tests/Query/Container/TruncatedCollectionOfTTest.cs:55
- [nitpick] Test method naming is inconsistent with the existing
CtorTruncatedCollection_SetsPropertiespattern. Consider standardizing on aCreate_*_SetsPropertiesorCtor_*_SetsPropertiesconvention for clarity.
public void TruncatedCollectionCreateForIEnumerable_SetsProperties()
src/Microsoft.AspNetCore.OData/Query/Container/TruncatedCollectionOfT.cs:189
- The XML doc comment has a duplicated
/// ///. It should be corrected to a single/// <param...>for proper tooling support.
/// /// <param name="totalCount">The total count. Default null.</param>
src/Microsoft.AspNetCore.OData/Query/Container/TruncatedCollectionOfT.cs
Show resolved
Hide resolved
src/Microsoft.AspNetCore.OData/Query/Container/TruncatedCollectionOfT.cs
Outdated
Show resolved
Hide resolved
…tionOfT.cs Co-authored-by: Copilot <[email protected]>
…OData/AspNetCoreOData into fix/improve-truncatedcollection
| /// <summary> | ||
| /// Used to track the truncation state of an async enumerable. | ||
| /// </summary> | ||
| public class TruncationState |
There was a problem hiding this comment.
Why have this a separate class instead of a boolean field of the TruncatedAsyncEnumerable class?
3 participants
No description provided.