Skip to content

ClientModel: Add SSE result collection#43821

Closed
annelo-msft wants to merge 16 commits intoAzure:mainfrom
annelo-msft:clientmodel-sseresultcollection
Closed

ClientModel: Add SSE result collection#43821
annelo-msft wants to merge 16 commits intoAzure:mainfrom
annelo-msft:clientmodel-sseresultcollection

Conversation

@annelo-msft
Copy link
Copy Markdown
Contributor

No description provided.

@azure-sdk
Copy link
Copy Markdown
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

System.ClientModel

/// <param name="response">The <see cref="PipelineResponse"/> received
/// from the service.</param>
protected ClientResult(PipelineResponse response)
protected ClientResult(PipelineResponse? response)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would only include this change if we have good reason to believe we'll want it for pageable/operation types in the future.

public abstract IAsyncEnumerator<T> GetAsyncEnumerator(CancellationToken cancellationToken = default);

// TODO: take CancellationToken?
//public static ClientResultCollection<T> Create<TValue>(PipelineResponse response) where TValue : IJsonModel<T>
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: could we accomplish this method signature?


public abstract IAsyncEnumerator<T> GetAsyncEnumerator(CancellationToken cancellationToken = default);

// TODO: take CancellationToken?
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Take cancellation token in factory method?

/// to the response most recently returned from the service.</remarks>
/// <param name="response">The <see cref="PipelineResponse"/> to return
/// from <see cref="GetRawResponse"/>.</param>
protected void SetRawResponse(PipelineResponse response)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Assert not null per nullability annotation


namespace System.ClientModel;

// TODO: Re-enable sync version
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: sync result collection


internal sealed class AsyncServerSentEventEnumerator : IAsyncEnumerator<ServerSentEvent>, IDisposable, IAsyncDisposable
{
// TODO: make this configurable per coming from TypeSpec
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: parameterize terminal event

return new(response, GetServerSentEventDeserializationEnumerator<U>);
}

private static IAsyncEnumerator<U> GetServerSentEventDeserializationEnumerator<U>(Stream stream, CancellationToken cancellationToken = default)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious: Why a func?


#region Helpers

private string _mockContent = """
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: put it in one place? Currently copied a lot.
Also: more robust set of test cases?

@annelo-msft
Copy link
Copy Markdown
Contributor Author

Closing in favor of #43840

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants