[1.10.x] Properly encode model and dataclass default for schema#4781
Conversation
|
CI seems to be failing with installation error unrelated to the changes. |
|
I created #4782 to fix the CI problem. |
|
Perhaps, I can rebase |
|
FYI, I closed #4782 because it was on main and seems not a proper fix |
|
As the problem got fixed in 78ca710, you can rebase your patch now! |
3097b85 to
74ceaaf
Compare
|
Please review :) |
| return t(*seq_args) if is_namedtuple(t) else t(seq_args) | ||
| elif isinstance(dft, dict): | ||
|
|
||
| if isinstance(dft, dict): |
There was a problem hiding this comment.
Yes. Notice the change above: we transform models and dataclasses to dict in first if and need this to be a separate one to work.
There was a problem hiding this comment.
Any ideas how to make this more explicit and obvious, btw?
There was a problem hiding this comment.
maybe just a comment with the above explanation.
There was a problem hiding this comment.
I would add the first if as a separate if and move the isinstance(dft, dict) check to the top.
Here is my suggestion(I haven't tested it locally):
def encode_default(dft: Any) -> Any:
from .main import BaseModel
if isinstance(dft, BaseModel) or is_dataclass(dft):
dft = cast('dict[str, Any]', pydantic_encoder(dft))
if isinstance(dft, dict):
return {encode_default(k): encode_default(v) for k, v in dft.items()}
elif isinstance(dft, Enum):
return dft.value
elif isinstance(dft, (int, float, str)):
return dft
elif isinstance(dft, (list, tuple)):
t = dft.__class__
seq_args = (encode_default(v) for v in dft)
return t(*seq_args) if is_namedtuple(t) else t(seq_args)
elif dft is None:
return None
else:
return pydantic_encoder(dft)There was a problem hiding this comment.
Happy to accept suggestions on phrasing as well :)
Previous suggestion
| if isinstance(dft, dict): | |
| # we can end up here either from first condition or if none of above conditions were met | |
| if isinstance(dft, dict): |
There was a problem hiding this comment.
Thanks for suggestion, @hramezani, I like your approach! I think I was hesitant on reordering at first, but it seems like the best move here, indeed.
|
@Bobronium I left a suggestion and asked a question. Please update |
Co-authored-by: Hasan Ramezani <hasan.r67@gmail.com>
|
Nice catch with suggestion, @hramezani! I answered the question as well. Please review. |
| return t(*seq_args) if is_namedtuple(t) else t(seq_args) | ||
| elif isinstance(dft, dict): | ||
|
|
||
| if isinstance(dft, dict): |
There was a problem hiding this comment.
maybe just a comment with the above explanation.
As per suggestion in pydantic#4781 (comment).
* Properly encode model and dataclass default for schema * Wrap type annotation in quotes * Fix compatibility * Add changes file * Remove unnecessary import Co-authored-by: Hasan Ramezani <hasan.r67@gmail.com> * Add a blank line back * Reorder conditions As per suggestion in #4781 (comment). Co-authored-by: Hasan Ramezani <hasan.r67@gmail.com>
|
Thanks so much. I've also cherry-picked this into |
* Properly encode model and dataclass default for schema * Wrap type annotation in quotes * Fix compatibility * Add changes file * Remove unnecessary import Co-authored-by: Hasan Ramezani <hasan.r67@gmail.com> * Add a blank line back * Reorder conditions As per suggestion in pydantic/pydantic#4781 (comment). Co-authored-by: Hasan Ramezani <hasan.r67@gmail.com>
Closes #4774
Fix was pretty straightforward, I guess we can have this in 1.10.x.