Make fields with defaults not required in the serialization schema by default #7275
Conversation
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
|
please review |
Kludex
approved these changes
Aug 29, 2023
Deploying with
|
| Latest commit: |
475fc07
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f34f93d4.pydantic-docs2.pages.dev |
| Branch Preview URL: | https://dmontagu-serialization-defau.pydantic-docs2.pages.dev |
dmontagu
commented
Sep 15, 2023
dmontagu
commented
Sep 15, 2023
|
|
||
| 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.
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.
dmontagu
commented
Sep 15, 2023
| @@ -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.
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. |
Kludex
approved these changes
Sep 18, 2023
docs/usage/model_config.md
Outdated
|
|
||
| ## JSON schema customization | ||
|
|
||
| #### `json_schema_serialization_defaults_required` |
There was a problem hiding this comment.
There's a limit on characters. Also, I don't think we should use parameter name as the name of the section.
davidhewitt
added
the
relnotes-change
hathawsh
added a commit
to OpenPaymentNetwork/openapi-pydantic
that referenced
this pull request
Oct 12, 2023
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #7209
Note this PR is not against the
mainbranch, 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