-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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: resolve forward refs for inherited dataclasses #2220
fix: resolve forward refs for inherited dataclasses #2220
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2220 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 4199 4200 +1
Branches 854 854
=========================================
+ Hits 4199 4200 +1
Continue to review full report at Codecov.
|
@PrettyWood thanks for the PR. But I believe it doesn't resolve the original issue as I was using pydantic dataclasses, not stdlib ones. |
Ah yes thanks for double checking @laevilgenius you're absolutely right! # for class A
namespace == {'__annotations__': {'uuid': 'UUID'}, '__module__': 'a', 'uuid': Ellipsis}
# for class B
namespace == {'__annotations__': {'uuid': 'UUID'}, '__module__': '__main__', 'uuid': Ellipsis} And with postponed evaluation of annotation, we don't have the right module to resolve Even though this PR still fixes one problem, I would like to tackle the whole "postponed evaluation" thing properly and I'm not convinced of my solution. I hence put the "deferred" label. |
As it doesn't solve the target issue, let's change the PR number
@PrettyWood don't we have the same problem on normal models inheriting from Frankly postponed annotations are a big head ache, I'm not all all clear if we can ever have a completely watertight solution, but since they'll become the default in 3.10 I guess we'll have to do our best. |
@samuelcolvin yes we do have the same problem hence the
I agree it's a mess but it's something we need to start thinking about. Not directly for v2 but to keep in mind! This PR can probably be closed TBH. But it was a good way for me to start playing with it and have a look at the PEP |
makes sense, but it looks like this is still an improvement? If you think it would be better to merge it, I can easily resolve conflicts and merge? |
Sorry for the wrong reply I was on my phone yesterday and answered too fast. We don't have the same issue with This PR fixes a problem and should probably be merged yes! |
Change Summary
When a stdlib
dataclass
is used inside aBaseModel
, it is converted into a pydanticdataclass
. But__annotations__
passed down when converted were not resolved properly so if we used'Literal...'
or postponed annotations by default withfrom __future__ import annotations
, it failed.The main problem is that we don't set
__module__ = _cls.__module__
in the pydanticdataclass
but rather__module__ = __name__
. This is done to allow pickling. So theresolve_annotations
done in the metaclass fails.To support both I suggest we simply call
resolve_annotations
when creating the new pydanticdataclass
Related issue number
Checklist
changes/<pull request or issue id>-<github username>.md
file added describing change(see changes/README.md for details)