-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add oneOf
+const
JSON Schema Option for Literals
#9029
Closed
Closed
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
f8b8d26
add 'literal_json_schema_type' config option
rmehyde 9ab3272
implement 'json_schema_literal_type' option
rmehyde 22f0e3d
add tests for 'json_schema_literal_type' option'
rmehyde f9242a3
cleanup docs
rmehyde 3bf1614
fix logic and add additional test for 3.9 and 3.10
rmehyde e02fb06
skip listenum test for 3.8
rmehyde f3f0a24
move literal type option from model config to model_json_schema() par…
rmehyde 7251376
whitespace - retrigger cicd
rmehyde 7bdf02f
Revert "whitespace - retrigger cicd"
rmehyde File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains 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 file contains 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 file contains 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 file contains 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 file contains 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
Oops, something went wrong.
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.
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.
I'd been following a great suggestion to look to the newly added opt-in attribute docstring support for inspiration here.
But I'm realizing that I followed that paradigm a bit too closely by putting the config option here, I think the best place for it is in a
BaseModel.model_json_schema()
parameter. Will refactor.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.
@rmehyde,
Ah yeah, that makes sense re moving to
model_json_schema
. That being said, you should still be able to keep most / all of the docs / tests that you've written, which is nice. Ping me when you've refactored, and I'd be more than happy to review! Looks like great work so far :).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.
Thanks @sydney-runkle, I've made that update!
There's one check failing: a segfault when running the Ubuntu 3.10 tests. I don't believe this is directly related to my changes, and I don't get that behavior running the command in a 3.10 environment on my own Ubuntu machine. But I did re-run the CI/CD and the behavior is consistent, let me know what the right next step there is.
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.
All good, that test is annoyingly flaky. I just reran it again, hopefully it passes this time 🍀
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.
At a first glance, looks great. I'll get back to you with a final confirmation tomorrow regarding if this is the best place to have this parameter. I think it is, but I suppose I could see an argument for just creating a custom instance of
GenerateJsonSchema
with this logic, and passing that to themodel_json_schema()
function.Could you re-add that comprehensive docs section that you added? Maybe to the JSON schema docs (instead of where you originally had it, in the config docs)