Incorporate further ApiView feedback #42
Conversation
| @@ -23,7 +23,7 @@ public partial class Assistant | |||
| /// <item><b>Values</b> can be a maximum of 512 characters in length.</item> | |||
| /// </list> | |||
| /// </remarks> | |||
| public IReadOnlyDictionary<string, string> Metadata { get; } | |||
| public IReadOnlyDictionary<string, string>? Metadata { get; } | |||
There was a problem hiding this comment.
in .NET collections should never be null. The guidelines say they should be empty
There was a problem hiding this comment.
Thanks! This is likely 100% intended, but just to explicitly check: since OpenAI provides empty collections on some response array properties and null on others, us standardizing means we'll lose any parallel distinction. I'm not aware of any case where there's the potential for a "minimal pair" with null and empty meaning different things (and I'd be very sad if we found one), but I can't categorically rule out the possibility, either. Do you have any concerns on that front? If not, I'll ensure we're empty across the board.
There was a problem hiding this comment.
PR updated with a pass across collections.
| { | ||
| GeneratedImage IJsonModel<GeneratedImage>.Create(ref Utf8JsonReader reader, ModelReaderWriterOptions options) | ||
| { | ||
| var format = options.Format == "W" ? ((IPersistableModel<GeneratedImage>)this).GetFormatFromOptions(options) : options.Format; |
There was a problem hiding this comment.
these 5 lines of code are repeated a lot. Should we have one helper somewhere?
There was a problem hiding this comment.
I'll abstract it. Most of it will hopefully move back into generated code in the not-too-distant term, but we're still going to have custom implementations and there's no good reason to not de-dupe it.
There was a problem hiding this comment.
PR updated, the internal static ModelSerializationHelpers class abstracts the repeated patterns across IJsonModel<T> and IPersistableModel<T>.
| Text = text; | ||
| Words = words; | ||
| Segments = segments; | ||
| Text = text!; |
There was a problem hiding this comment.
Should Text be declared as nullable here? Why use the null-forgiving operator (!)?
| @@ -141,13 +124,13 @@ public ImageClient(string model, ApiKeyCredential credential = default, OpenAICl | |||
|
|
|||
| options ??= new(); | |||
|
|
|||
| using MultipartFormDataBinaryContent content = options.ToMultipartContent(fileStream, fileName, prompt, _clientConnector.Model, imageCount); | |||
| using MultipartFormDataBinaryContent content = options.ToMultipartContent(fileStream, fileName, prompt, _clientConnector.Model!, imageCount); | |||
There was a problem hiding this comment.
Curious on this one -- how do we know _clientConnector.Model will never be null? If it won't ever be null, should its type be non-nullable?
| : base(ChatRole.Assistant, content) | ||
| /// <param name="text"> Optional text content associated with the message. </param> | ||
| public ChatRequestAssistantMessage(ChatFunctionCall functionCall, string? text = null) | ||
| : base(ChatRole.Assistant, text) |
There was a problem hiding this comment.
I think it's preferable to call it "content" like we did originally, because it's not that we're being inconsistent, but rather that we're representing a subtle but arguably important distinction. This is how I think about it:
- All the different kinds of message have some form of "content", and we should consistently refer to it as such, regardless of how it is represented (string, array, etc.). This is important in order to align with the REST API and the documentation.
- In some cases, the content can be an array of "content parts", where a content part can be plain text, an image URL, and possibly other things in the future. We should consistently use "text" when talking about "plain text content parts".
What do you think?
|
Closing this PR in favor of the replacements mentioned above -- those pull apart the aims of this one better informed by the further discussion we've had. |
This PR touches many files, but the volume comes primarily from bulk pattern application.
Changes include:
Nameproperties have been been updated to use a more specific name -- eitherFunctionNameorParticipantName-- in cases where the question of "what is this the name of" isn't trivially inferable from the type.initfor types whererequired/initare an intended initialization pattern.ChatMessageContent's type conversions tostringand (new)Uriare now explicit.ChatRequestAssistantMessagenow uses "text" for its content constructor parameter name.CreatedAtis now present onGeneratedImageCollection(in addition to its emplacement onGeneratedImage).ImageQualityandImageStyleare renamed with the consistentGenerated*prefix.OpenAIClientOptionsnow derives from the appropriatePipelineClientOptionstype.GetLegacyCompletionClientis removed fromOpenAIClient.