MPFD Generated Proposal#6415
Conversation
microsoft-github-policy-service
Bot
added
the
emitter:client:csharp
jorgerangel-msft
force-pushed
the
mpfd-pet-design-doc
branch
from
5c767ae to
504abd6
Compare
|
No changes needing a change description found. |
microsoft-github-policy-service
Bot
added
the
stale
jorgerangel-msft
removed
the
stale
jorgerangel-msft
force-pushed
the
mpfd-pet-design-doc
branch
from
e5ab0a4 to
d23af9d
Compare
jorgerangel-msft
force-pushed
the
mpfd-pet-design-doc
branch
from
d23af9d to
22f9156
Compare
| { | ||
| public MultiPartFormDataBinaryContent() { } | ||
| public MultiPartFormDataBinaryContent(string boundary) { } | ||
| public void Add(string name, System.BinaryData content, string? mediaType = null) { } |
There was a problem hiding this comment.
what if there is a conflict between the BinaryData.MediaType property and the mediaType parameter?
There was a problem hiding this comment.
That's a good point that I didn't consider. We could just default to the mediaType of the BinaryData content, unless it's explicitly overwritten by the mediaType parameter in this method.
edit: actually, since the overload that takes in FileBinaryContent doesn't specify mediaType as a parameter I'm leaning towards just removing mediaType from this method as well and have the mediaType parsed from the BinaryData instance.
| public MultiPartFormContent() { } | ||
| public MultiPartFormContent(string boundary) { } | ||
| public void Add(string name, System.BinaryData content) { } | ||
| public void Add(string name, bool content, string? mediaType = "text/plain") { } |
There was a problem hiding this comment.
is text/plain the right default for bools? should all the default media types be JSON? except for the byte[] one
There was a problem hiding this comment.
@KrzysztofCwalina hmm, the rfc guidelines recommend defaulting to text/plain and both swagger and typespec follow this default handling for the parts. With that being said, could you help me understand why we should default to JSON? It makes sense to me for complex values.
There was a problem hiding this comment.
what does it mean to write bool as text? what do we write? the english word "true"/"false", 0 or 1? Same with numbers and other data types? what is the text representation of double 1000.29? is it "1,000.29" or "1000,29"? these are both textual representations of the number in different locales.
There was a problem hiding this comment.
@KrzysztofCwalina - ah I see what you're saying. I think the missing implementation detail here is that with the exception of byte[] and BinaryData parts, StringContent is being used to serialize the part. So a "bool" part is being serialized as "true" / "false". The implementation is very similar to the one used in OAI which I used as a baseline.
With that being said, if you think we should always default to json, I can look into how that implementation would look.
There was a problem hiding this comment.
@jorgerangel-msft, can you point me to code showing how StringContent is used to "serialize" bools and numbers. I dont think StringCOntent has any APIs to serialize bools/numbers.
There was a problem hiding this comment.
@KrzysztofCwalina - no you're right it doesn't. In the implementation for this type, bools + numbers are converted to a string and then StringContent is used similar to https://github.com/openai/openai-dotnet/blob/main/src/Generated/Internal/MultiPartFormDataBinaryContent.cs#L100
There was a problem hiding this comment.
Yeah, mydecimal.ToString("G", CultureInfo.InvariantCulture) is unlikely to be the right thing. Sending numbers in some .NET specific format (e.g. "G") over the wire is probably not very useful as the server would have to know about this unwritten "protocol".
BTW, do we really have scenarios where the part content is just one number or boolean? If yes, what are those?
There was a problem hiding this comment.
@KrzysztofCwalina - I see. I will default to json for the parts then and look into changing the implementation.
I haven't ran into scenarios where a part is a single bool / number. It's supported by tsp, so I added it as an overload since this will be consumed by the generator. Though I'm also fine with removing them from this type for now.
There was a problem hiding this comment.
Sounds good. Let's go with JSON and separately you might want to chat with the typespec team about the scenarios. I really wonder if there are any for sending a single bool, int, etc. as a form part
microsoft-github-policy-service
Bot
added
the
stale
jorgerangel-msft
removed
the
stale
microsoft-github-policy-service
Bot
added
the
stale
jorgerangel-msft
removed
the
stale
microsoft-github-policy-service
Bot
added
the
stale
commit: |
3 participants
No description provided.