Support recursive dataclasses #5760
Conversation
|
please review |
pydantic/_internal/_dataclasses.py
Outdated
| if _parent_namespace_depth > 0: | ||
| frame_parent_ns = _typing_extra.parent_frame_namespace(parent_depth=_parent_namespace_depth) or {} | ||
| # cls_parent_ns = cls.__pydantic_parent_namespace__ or {} | ||
| # cls.__pydantic_parent_namespace__ = {**cls_parent_ns, **frame_parent_ns} |
There was a problem hiding this comment.
Not sure if we want to store the original namespace on the dataclasses like we do for BaseModel.
This helps in the scenario where rebuilds are necessary when importing from a separate module and there was a ForwardRef to a type in the module in which the class was originally defined.
This may also be necessary when parametrizing generic cyclically-recursive dataclasses outside the module in which they were defined.
I've left these lines present and commented as a reminder to decide about this. I'm okay either way.
Note: related, it may also be a good idea to make the _types_namespace argument here and/or on BaseModel "public". (For context: It currently has the leading _ because we wanted to discourage its usage because picking up references from the current namespace should generally work, but we still included the argument in BaseModel.model_rebuild to make it possible to more explicitly control what ForwardRefs evaluate to if you know what you are doing.)
There was a problem hiding this comment.
I'm pretty neutral / would defer to you on it.
My preference is "simple wins", so I think I'd prefer not to store the namespace, but don't really mind.
There was a problem hiding this comment.
I had a chance to speak with Adrian about this briefly, and I think we agreed it would be nice to keep the behavior as similar as possible between BaseModel and pydantic dataclass.
I do see your point about keeping things simple, but I also vaguely recall that I had added that __pydantic_parent_namespace__ in the BaseModel case for the purpose of dealing with recursive generics.
Before proceeding to merge this branch, I'm going to try to work on top of this branch and get recursive generic dataclasses working. If it becomes clear that I will need the __pydantic_parent_namespace__ thing to get that to behave nicely, it might make this a moot point.
There was a problem hiding this comment.
Okay, given there is a lot of complexity related to generic dataclasses and how exactly we should handle them, and not handling it properly is not a regression from v1 (as it wasn't handled properly there), my new plan is to not fiddle with the parent namespace, and also not support generic dataclasses in v2 (but perhaps in v2.1, or maybe sooner if we happen to get it all working in time.. but the pressure is off.)
pydantic/_internal/_dataclasses.py
Outdated
| if _parent_namespace_depth > 0: | ||
| frame_parent_ns = _typing_extra.parent_frame_namespace(parent_depth=_parent_namespace_depth) or {} | ||
| # cls_parent_ns = cls.__pydantic_parent_namespace__ or {} | ||
| # cls.__pydantic_parent_namespace__ = {**cls_parent_ns, **frame_parent_ns} |
There was a problem hiding this comment.
I'm pretty neutral / would defer to you on it.
My preference is "simple wins", so I think I'd prefer not to store the namespace, but don't really mind.
Deploying with
|
| Latest commit: |
2ae81be
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d657bb80.pydantic-docs2.pages.dev |
| Branch Preview URL: | https://dataclasses-rebuild.pydantic-docs2.pages.dev |
|
please update. |
Closes #5721. This is also a precursor to work on generic dataclasses.
I'll open the PR now, but I think there's a variety of things to establish some consensus on before merging:
model-not-fully-definedand (analogous)dataclass-not-fully-definedusage errors?model-not-fully-definedand replaced it with a commonclass-not-fully-definederror, but would be happy to revert this.dataclass-not-fully-definedan acceptable name?model-not-fully-definedconsidering it's largely similar and we have a docs section for each usage error code?BaseModel.__pydantic_model_complete__toBaseModel.__pydantic_complete__and used the same property on pydantic dataclasses. Any objections?BaseModel.model_rebuildmethod. I have exported an analogous function calledpydantic.dataclasses.rebuild_dataclass.pydantic._internal._dataclassesand re-exported inpydantic.dataclasses. Is this acceptable?__pydantic_dataclass_rebuild__or similar to pydantic dataclasses — is that still the case? If so, what should the name of that method be?)I'll be happy to make any requested changes.
Selected Reviewer: @samuelcolvin