Skip to content

Conversation

@KrzysztofCwalina
Copy link
Collaborator

To improve debugging experience

@KrzysztofCwalina KrzysztofCwalina merged commit 83d6bef into main Jun 25, 2025
1 check passed
@KrzysztofCwalina KrzysztofCwalina deleted the ToString branch June 25, 2025 22:20
@joseharriaga
Copy link
Collaborator

Should we add something to make it clearer that this is for debugging purposes only? I feel like it could confuse users into doing things like:

  1. Displaying this string in some production UI.
  2. Forwarding this string as part of a turn-by-turn conversation instead of using the real collection of content parts with the real values returned by the LLM.

@joseharriaga
Copy link
Collaborator

Maybe we can return a string that is prefixed by: "DEBUG: ".

switch (part.Kind)
{
case ChatMessageContentPartKind.Text:
builder.Append(part.Text);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can potentially return an empty string, which is against the ToString guidelines.

break;

default:
builder.Append("<unknown content kind>");
Copy link
Collaborator

@joseharriaga joseharriaga Jun 25, 2025

Choose a reason for hiding this comment

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

nit: This should say "<unknown content part kind>" or simply "<unknown>"

@KrzysztofCwalina
Copy link
Collaborator Author

Maybe we can return a string that is prefixed by: "DEBUG: ".

No other ToString implementations do it.

Displaying this string in some production UI.

I think it's fine if they do. In fact I think we should do it in our samples. I know you are trying to avoid people writing incorrect code when they have images and such, but all the code I saw written by others (and us) does completion.Content[0].Text, which is even less correct.

@joseharriaga
Copy link
Collaborator

joseharriaga commented Jun 26, 2025

@KrzysztofCwalina

No other ToString implementations do it.

But other ToString implementations tend to follow the ToString guidelines and generally feel a little more objective than this one.

completion.Content[0].Text, which is even less correct.

I disagree that this is less correct. It implies that the way you handle content is dependent on the specifics of your scenario. As much as we wished Content was simpler than a collection of random things, that is how the REST API is implemented, and I think we're doing users a disservice by hiding that from them. In reality, no customer should use ToString in production in the way it is implemented here. Users must tailor their code to their specific scenario. This is increasingly important as new kinds of content part are added.

I agree that we need to improve debuggability. What do you think about implementing the functionality here using a custom view for the debugger via DebuggerBrowsable, DebuggerDisplay or something similar?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants