-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Make fields with defaults not required in the serialization schema by default #7275
Conversation
please review |
Deploying with Cloudflare Pages
|
|
||
This can be useful when using frameworks (such as FastAPI) that may generate different schemas for validation | ||
and serialization that must both be referenced from the same schema; when this happens, we automatically append | ||
`-Input` to the definition reference for the validation schema and `-Output` to the definition reference for the |
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.
The reason I removed the ability to override the -Input
and -Output
values was twofold:
- Right now, these refs are computed after the config has been removed from the config stack. It was possible to work around this, but very ugly
- I think the most compelling use case was to be able to set one of suffixes to be the empty string, e.g. so the input would use the empty string and output would use
'-Output'
. Unfortunately, the way I've implemented it, it tries to use the first definition reference that is not used by any other schema. Because the serialization schema tries to use without the suffix before using the suffix, it means the empty string just doesn't work, because there are always two schemas that want the nameModel
.
I believe with enough effort it may be possible to handle this better, but I think the changes still made by this PR are higher priority than controlling the definition ref suffix so I figured it was worth getting them in and deferring further work on this.
@@ -713,7 +713,7 @@ class Model(BaseModel): | |||
path.unlink() | |||
|
|||
path = Path('directory') | |||
path.mkdir() | |||
path.mkdir(exist_ok=True) |
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.
Unrelated to this PR, but this resolved some local test failures for me, and I don't see any reason not to keep it there.
Assigning myself since I'm already in the loop here. |
docs/usage/model_config.md
Outdated
|
||
## JSON schema customization | ||
|
||
#### `json_schema_serialization_defaults_required` |
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.
Closes #7209
Note this PR is not against the
main
branch, but against @Kludex's branch for fixing the JSON schema generation based on config settings as it shares some implementationThis should have tests added for the new config settings before being merged into
main
, but I wanted to open now to get some initial review. Reading the discussion in the issue linked above may be helpful for understanding the motivation for this change.Selected Reviewer: @lig