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
Fix schema for set and frozenset with default value #4155
Fix schema for set and frozenset with default value #4155
Conversation
please review |
@@ -984,7 +984,7 @@ def encode_default(dft: Any) -> Any: | |||
return dft.value | |||
elif isinstance(dft, (int, float, str)): | |||
return dft | |||
elif sequence_like(dft): | |||
elif isinstance(dft, (list, tuple)): |
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'm really not into the code base - so this is just a non-constructive critisism, but this change would narrow the scope compared to the original sequence_like check. What happens when these instances (set, frozenset, ...) are passed in?
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'm also very new to the codebase, but I think the sequence_like
usage is not very accurate here.
If you look further, after these conditions it will try to use the pydantic_encoder for all unhandled types which in this case is set and frozenset.
And also there are tests for all cases and new ones for set and frozenset, so I think this is not narrowing down any type, just setting up different encoders for these types.
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.
sounds reasonable! now we only need @samuelcolvin or @PrettyWood to approve :-)
Thanks @aminalaee for this patch 👍 LGTM I think it would be good to add |
please update |
…rialize-set-with-default-values
please review |
Thanks so much @aminalaee. |
Change Summary
Fixes JSON schema for
set
andfrozenset
when they have default values, the output json is not valid and cannot be serialized.For example:
It will produce:
But instead should be:
Related issue number
Fixes #4136
Checklist
changes/<pull request or issue id>-<github username>.md
file added describing change(see changes/README.md for details)