Skip to content

Conversation

@alonsopf
Copy link

No description provided.

@festo
Copy link

festo commented Dec 28, 2024

@samlown @alonsopf This is what I was looking for, do we have a timeline for when this will be merged?
I know go is strongly typed but this could be useful when defining nullable fields. Eg OpenAI requires this

Although all fields must be required (and the model will return a value for each parameter), it is possible to emulate an optional parameter by using a union type with null

https://platform.openai.com/docs/guides/structured-outputs#all-fields-must-be-required

Copy link
Contributor

@samlown samlown left a comment

Choose a reason for hiding this comment

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

This is a great start! Thanks! I don't think breaking changes should be made however without careful consideration.

Also, the tests have been updated with the new structure, but I don't see any actual tests for the new functionality, specifically the parsing the output of the type arrays. There would need to be full test coverage for this PR to be accepted.

PropertyNames *Schema `json:"propertyNames,omitempty"` // section 10.3.2.4
// RFC draft-bhutton-json-schema-validation-00, section 6
Type string `json:"type,omitempty"` // section 6.1.1
Type []string `json:"type,omitempty"` // section 6.1.1
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is a significant breaking change for anyone using the Type field directly, and as such, I'm not convinced this is the correct approach.

I'm wondering if an alternative approach, given how rare this field is even used, might be to automatically handle a comma or semicolon separated list of types alongside the automatic conversion in the JSON marshalling methods.

A definition would effectively look like:

jsonschema.Schema{
  Type: "string,null",
}

or in a struct:

type Object struct {
  Text string `jsonschema:"type=string,null"`
}

Copy link

Choose a reason for hiding this comment

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

For the structure, we could even get it from the property type or the json tag. If it is a pointer or the json tag has the omitempty the null could be added to the jsonchema array

Copy link

Choose a reason for hiding this comment

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

@festo recognizing the pointer would be the easiest and most go-friendly. I like the idea!

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.

5 participants