Added CancellationToken parameters to the most complete convenience overloads. #53
Conversation
| @@ -915,7 +980,4 @@ private static ClientResult<T> CreateResultFromProtocol<T>(ClientResult protocol | |||
| T deserializedResultValue = responseDeserializer.Invoke(pipelineResponse); | |||
| return ClientResult.FromValue(deserializedResultValue, pipelineResponse); | |||
| } | |||
|
|
|||
| private static RequestOptions StreamRequestOptions => _streamRequestOptions ??= new() { BufferResponse = false }; | |||
| private static RequestOptions _streamRequestOptions; | |||
| } | |||
There was a problem hiding this comment.
If Ctrl+C/Ctrl+V aren't worn out on the keyboard yet, could we also mirror this in the companion AssistantClient.Convenience.cs file's overloads? I know we have a pending discussion item on whether we should even have those, but in the interim it'd be great to retain parity if at all possible.
There was a problem hiding this comment.
I don't think we want to do that. These are just simple call throughs, and if I add more and more overloads, the possibility of ambiguities at the call sites just goes up. That's why the title of the PR says "most complete convenience overloads".
There was a problem hiding this comment.
I noticed that, for example, in the AudioClient, the methods that take a Stream do have a CancellationToken, but the overloads that take a string file path do not. Also noticed the same with other similar overloads in the FileClient and the ImageClient. I think this is a little unexpected because it forces users that need a CancellationToken into a specific paradigm.
If the main concern of adding CancellationToken is that things are more likely to break due to ambiguities in the future, I wonder if there are mitigation strategies that we could consider. Off the top of my head, what if any client method only ever had two optional parameters: The CancellationToken parameter and an options class parameter (and then the options class holds all the other optional parameters, present and future). I recognize that preemptively adding an options class for methods that only have one or two optional parameters today is perhaps not ideal, but personally I think that the benefit of knowing that this would never break due to ambiguities in the future is a good tradeoff.
There was a problem hiding this comment.
Yeah, let's look at the APIs wholistically and see if we want to add more of these overloads.
fixes #35