Skip to content

Conversation

4t145
Copy link
Contributor

@4t145 4t145 commented Jul 1, 2025

Remain original response body, so users can do provider specific operations.

@4t145 4t145 changed the title feat: remain original reponse body feat: remain original response body Jul 1, 2025
@jeremychone
Copy link
Owner

@4t145 So, this is a good advanced feature to have.

Now, for the implementation, we would need a few things.

  1. It should not be enabled by default, and should be behind ClientConfig, probably .capture_raw_body.
  2. In the ChatResponse, it will be capture_raw_body: Option<serde_json::Value>.
  3. The body clone should happen first, before the "remove" happens.

So, we have two options.

a) Close this PR and create a new one with just one commit. I really need to see all of the changes in one commit so I don't have to merge everything in my head.

b) I merge this PR, and then make the change myself in another commit.

let me know how you want to proceed.

@jeremychone jeremychone added the in-progress This ticket is in the work, first commit might have been put in master. label Jul 1, 2025
@jeremychone
Copy link
Owner

@4t145 so, you want the option b) or a) ?

@4t145
Copy link
Contributor Author

4t145 commented Jul 1, 2025

I can merge all the commits into one, but if you are willing to implement those yourself, you can just merge this and modify it your self. Otherwise I will handle this tomorrow. Thanks for your work!

@4t145 4t145 force-pushed the feat-remain-original-response-body branch from 72cd570 to 75c4320 Compare July 2, 2025 06:21
@4t145
Copy link
Contributor Author

4t145 commented Jul 2, 2025

@jeremychone Now you can review again.

@jeremychone
Copy link
Owner

@4t145 Cool, will look at it tomorrow or Friday.

@jeremychone
Copy link
Owner

@4t145 Ok, overall looks good. One thing, is that you added "client" to the to_chat_response to get th options, but we alreayd have the option_sets.

I will merge this PR and fix it.

Also, the property to get the content should be captured_raw_body.

But otherwise, pretty good. I like the way you did let capture_raw_body = client.config().capture_raw_body().then(|| body.clone());

@jeremychone jeremychone merged commit cf979dc into jeremychone:main Jul 3, 2025
@jeremychone
Copy link
Owner

I understand the confusion. You put it in ClientConfig, which is understandable. But we want this at the Chat Options level so hat we can override it per chat.

jeremychone added a commit that referenced this pull request Jul 3, 2025
@jeremychone
Copy link
Owner

@4t145 Done. So, now you set it at the ChatOptions level. Which can be set at the client level, or when making a request (and it will override)

@jeremychone jeremychone added the PR-MERGED Added for PR that somehow did not get the Merge label Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in-progress This ticket is in the work, first commit might have been put in master. PR-MERGED Added for PR that somehow did not get the Merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants