Skip to content
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

Add Pipeline Types to System.ClientModel library #41085

Merged
merged 22 commits into from
Jan 12, 2024

Conversation

annelo-msft
Copy link
Member

@annelo-msft annelo-msft commented Jan 8, 2024

Add basic pipeline types to the System.ClientModel library, including:

  1. ClientPipeline
  2. ClientPipelineOptions
  3. PipelinePolicy
  4. PipelineTransport
  5. PipelineMessage
  6. PipelineRequest
  7. BinaryContent

A discussion of the types contained in this PR can be found in the System.ClientModel README.

@azure-sdk
Copy link
Collaborator

azure-sdk commented Jan 8, 2024

API change check

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

System.ClientModel

Copy link
Member

@KrzysztofCwalina KrzysztofCwalina left a comment

Choose a reason for hiding this comment

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

I am approving the PR, but we should cleanup the ifdefs and the _writer field (issues are filed) before we GA.

@annelo-msft annelo-msft enabled auto-merge (squash) January 12, 2024 17:06
@annelo-msft annelo-msft merged commit c61d44d into Azure:main Jan 12, 2024
32 checks passed
@annelo-msft
Copy link
Member Author

Since this merged as comments were still coming in on resolved threads, if there are any outstanding comments that didn't get addressed but are important and not covered by issues in the ClientModel backlog, please feel free to file an issue to track, or also you're welcome to put up a PR into main to address targeted areas of interest! Thanks!


public override async Task WriteToAsync(Stream stream, CancellationToken cancellation)
{
if (_model is IJsonModel<T> && _options.Format == "J")
Copy link
Member

Choose a reason for hiding this comment

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

I think we should perhaps expose this functionality in an internal static method on MRW so that we can duplicate the logic. It will have a higher hit rate on the faster API which will be a good thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Per offline discussion, will address in a separate PR, and thanks for raising, @m-nash!

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.

8 participants