Adding AsyncCollection<T> for Storage enumeration #6767
Conversation
tg-msft
requested review from
pakrym,
KrzysztofCwalina,
kfarmer-msft and
seanmcc-msft
| /// iterate over. | ||
| /// </summary> | ||
| /// <typeparam name="T">The type of the values.</typeparam> | ||
| public abstract class AsyncCollection<T> : IAsyncEnumerable<Response<T>>, IEnumerable<Response<T>> |
There was a problem hiding this comment.
I wonder if we need IEnumerable<Response> on this type at all. I can be implemented on StorageAsyncCollection
There was a problem hiding this comment.
I think we want this on the main public type. Even if it's implemented explicitly, customers should be able to see it's supported.
There was a problem hiding this comment.
It's an easy way to slip into sync codepath even after calling an async client method.
There was a problem hiding this comment.
That's a fair point - Enumerable extension methods are going to make this pretty easy for people to mess up. What are your thoughts @KrzysztofCwalina?
There was a problem hiding this comment.
Changing to hide the implementation of IEnumerable<T> which is a better bet until we decide on the Sync half of the story, as discussed.
| /// Initializes a new instance of the <see cref="AsyncCollection{T}"/> | ||
| /// class for mocking. | ||
| /// </summary> | ||
| protected AsyncCollection() => |
There was a problem hiding this comment.
super nit: Let's remove this ctor. Everyone should have a cancellation token when creating instances of this type.
There was a problem hiding this comment.
We don't want this even for mocking?
There was a problem hiding this comment.
Good point, I wonder if people would want to implement this as a mock, maybe we should just give them static factory methods.
AsyncCollection.Create(Response<T>[])
AsyncCollection.Create(Page<T>[])
| { | ||
| // TODO: Should this be Items instead of Values? | ||
| // - Probably not with Response.Values | ||
| // TODO: Should it be IEnumerable<T>? |
tg-msft
force-pushed
the
async-coll
branch
2 times, most recently
from
147e37b
to
89ff649
Compare
| public virtual Response<BlobsFlatSegment> ListBlobsFlatSegment( | ||
| string marker = default, | ||
| BlobsSegmentOptions? options = default, | ||
| public virtual IEnumerable<Response<BlobItem>> GetBlobs( |
There was a problem hiding this comment.
I think we should return a concrete type, as we talked. But then, we need a name for it, i.e. we probably cannot return Collection (for one, it's not readonly). Let's think about it after preview 1.
| /// <see cref="BlobContainerClient.GetBlobsByHierarchyAsync"/> | ||
| /// operations. | ||
| /// </summary> | ||
| public struct GetBlobsOptions : IEquatable<GetBlobsOptions> |
There was a problem hiding this comment.
This seems like a big struct and it's mutable. Maybe we should change it to a class.
There was a problem hiding this comment.
It's not that big once we remove PageSizeHint. It gets even smaller if we change the booleans to be a [Flags] enum.
| /// or previous <see cref="Specialized.BlobClient.StartCopyFromUriAsync"/> | ||
| /// operation should be included. | ||
| /// </summary> | ||
| public bool IncludeCopyMetadata { get; set; } |
There was a problem hiding this comment.
Should all these booleans just be a flags enum property? Just like in ConfigurationcLient APIs.
There was a problem hiding this comment.
I think that's reasonable and I'll change it if we add a guideline.
- Move shared code into folders - Remove unused constants
- Remove IStorageCredentials interface - Remove old TokenCredentials - Rename SharedKey => StorageSharedKey - Move PipelinePolicy into .Common - Switch Tests to the Identity library
- Move HttpRange into Common - Rename ID to Id - Rename cancellation to cancellationToken - Move access conditions into .Models - Move remaining options into .Models - Change Enqueue to return a single item instead of an array
- Move MessagesClient and MessageIdClient into QueueClient - Add DequeuedMessage.Update helper
We're temporarily ignoring an issue with the auto-sync testing generated proxy Azure#6716
(Please only look at the latest commit.)