-
Notifications
You must be signed in to change notification settings - Fork 292
Add support for dataclass fields init #1163
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
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1163 +/- ##
=========================================
+ Coverage 0 89.97% +89.97%
=========================================
Files 0 106 +106
Lines 0 16618 +16618
Branches 0 36 +36
=========================================
+ Hits 0 14952 +14952
- Misses 0 1659 +1659
- Partials 0 7 +7
... and 101 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
CodSpeed Performance ReportMerging #1163 will improve performances by 20.58%Comparing Summary
Benchmarks breakdown
|
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.
Other than the note I just added above, this looks good to me, though as discussed let's get others' feedback before merging. (I can't approve because technically I opened the PR but thank you for fixing handling of defaults and adding the tests @sydney-runkle).
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.
The proposed behaviour for extra='ignore'
seems consistent with current model behaviour, agreed. Is there a world where we consider defaults to be extra='forbid'
, e.g. in V3, which would allow us to align everything with stdlib dataclasses?
Horrible (nonsensical) edge case: what if init=False
and init_only=True
?
It looks like dataclasses
currently just fails in this case:
>>> @dataclasses.dataclass
... class Foo:
... f: dataclasses.InitVar[int] = dataclasses.field(init=False)
... def __post_init__(self):
... pass
...
>>> Foo()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "<string>", line 3, in __init__
NameError: name 'f' is not defined
We could try to fail with a nice error, or we could make this configuration do nothing, or maybe with the case of extra='ignore'
we could allow this variable to pass through to __post_init__
(and __init__
?) given that extra='ignore'
doesn't reject arbitrary keywords off __init__
.
Yuck 😂
Co-authored-by: David Hewitt <[email protected]>
Co-authored-by: David Hewitt <[email protected]>
We could disallow this at schema build time, like I've done here for the |
For now disallowing seems good, we can always relax that in future without breaking backwards compatibility. |
We might want more careful error handling in certain circumstances. In particular, the default behavior of pydantic dataclasses of ignoring extra could end up being confusing for people who don't realize what
init=False
is supposed to do, as this implementation results in it being ignored the same as any other attribute. I do think this is the most "consistent" behavior with other functionality, and that if you want it to be an error to pass the value it should probably have theextra='forbid'
set in the config. Open to disagreement though.Also, needs tests added.