-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Move async collection to core #7038
Conversation
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.
Added a couple open questions, but nothing that needs to block this PR. Looks good!
/// iterate over. | ||
/// </summary> | ||
/// <typeparam name="T">The type of the values.</typeparam> | ||
public abstract class AsyncCollection<T> : IAsyncEnumerable<Response<T>> |
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.
Open question - are we going to have a SyncCollection<T>
or similar as an indicator to customers that the sync version of GetBlobs()
may make 100 requests?
/// <summary> | ||
/// Gets the values in this <see cref="Page{T}"/>. | ||
/// </summary> | ||
public IReadOnlyList<T> Values { get; } |
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.
Open question - we've got open issues like #6894 to use T[]
(rather than IEnumerable<T>
or IReadOnlyList<T>
) and it'd be great to be consistent.
Fixes: #6807
I also cleaned up some todos:
Values seem fine considering
Response.Value
I doubt we ever going to lazily deserialize items and the array is easy to consume unfortunately it's mutable so I've picked
IReadOnlyList
as a middle ground.I think it would add complexity around serializing it into a string that would fall onto customers. Keeping as a string.