-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: properly unmarshal JSON schema in ChatCompletionResponseFormatJSONSchema.schema #1028
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1028 +/- ##
==========================================
+ Coverage 85.83% 85.97% +0.13%
==========================================
Files 43 43
Lines 2316 2338 +22
==========================================
+ Hits 1988 2010 +22
Misses 305 305
Partials 23 23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@eiixy thanks for opening a PR for this. I tested these change but I am still seeing the following error :
I think issue stems from the Schema field in ChatCompletionResponseFormatJSONSchema being defined as an interface JSONSchema. Are you also seeing this error or am I missing something? |
Hey, I just pushed some new tweaks to address the issue you mentioned. Could you pull the latest code and give it another go? |
Its working now, changes lgtm. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures that ChatCompletionResponseFormatJSONSchema.schema
is properly unmarshalled into a jsonschema.Definition
and adds tests to validate the behavior.
- Introduces a custom
UnmarshalJSON
forChatCompletionResponseFormatJSONSchema
to handle object and null schema values correctly. - Adds unit tests for
ChatCompletionResponseFormatJSONSchema.UnmarshalJSON
and nestedChatCompletionRequest
unmarshalling.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
chat.go | Implements a custom UnmarshalJSON to correctly parse the schema field. |
chat_test.go | Adds tests for schema unmarshalling scenarios, including valid object, null , and invalid inputs. |
Comments suppressed due to low confidence (2)
chat_test.go:960
- [nitpick] Several test cases use empty strings for the
name
field, which can make it hard to identify failing subtests. Consider providing descriptive names for each scenario.
"",
chat_test.go:954
- The tests for
ChatCompletionResponseFormatJSONSchema.UnmarshalJSON
don't include a case where thedescription
field is present in the input. Adding a test that includes a non-emptydescription
will ensure that field is also unmarshalled correctly.
tests := []struct {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR!
…ONSchema.schema (sashabaranov#1028) * feat: sashabaranov#1027 * add tests * feat: sashabaranov#1027 * feat: sashabaranov#1027 * feat: sashabaranov#1027 * update chat_test.go * feat: sashabaranov#1027 * add test cases
When handling the response format for ChatCompletionResponseFormatJSONSchema, the field
response_format.json_schema.schema fails to unmarshal properly
Issue: #1027