feat: add warning for .optional() usage in OpenAI API schemas #1214
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds a warning when .optional() is used in schemas for OpenAI API Structured Outputs, recommending the use of .nullable() instead. - Added warning in optional.ts that triggers when openaiStrictMode is true - Added test to verify warning behavior Fixes openai#1180
1 task
|
|
||
| export const parseOptionalDef = (def: ZodOptionalDef, refs: Refs): JsonSchema7Type | undefined => { | ||
| if (refs.openaiStrictMode) { | ||
| const fieldName = refs.propertyPath?.slice(-1)[0] || 'unknown'; |
There was a problem hiding this comment.
Since refs.propertyPath is an array, can we replace .slice(-1)[0] with .at(-1) here? Using .at(-1) improves readability and performance by directly accessing the last element without creating a new array.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/at
| `See: https://platform.openai.com/docs/guides/structured-outputs#all-fields-must-be-required`, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Unnecessary use of string concatenation (+) since we are already using template literals.
| expect(consoleSpy).toHaveBeenCalledWith( | ||
| expect.stringContaining('uses .optional() which is not supported by OpenAI API Structured Outputs'), | ||
| ); | ||
|
|
There was a problem hiding this comment.
Could we also test expect(consoleSpy).toHaveBeenCalledTimes(1); to ensure this is called only once.
Collaborator
|
Thanks for the PR and sorry for the delayed response! I did this separately in #1469 as I wanted to take a slightly different approach. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add warning for .optional() usage in OpenAI API schemas
This PR adds a warning when
.optional()is used in schemas for OpenAI API Structured Outputs, recommending the use of.nullable()instead since optional fields are not currently supported.Changes
optional.tsthat triggers whenopenaiStrictModeis trueTesting
✅ All tests passing (411 total)
Implementation Details
The warning is implemented in the optional field parser and only triggers when generating schemas specifically for OpenAI API usage (when
openaiStrictModeis true). This ensures developers get immediate feedback about unsupported optional fields while keeping the warning relevant only to OpenAI API usage.Created with help from Devin: https://preview.devin.ai/devin/13e97f93e3b1445c91e60da01b88febd
Fixes #1180