Skip to content

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

Merged
merged 10 commits into from
Jan 17, 2024
Merged

Conversation

dmontagu
Copy link
Collaborator

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 the extra='forbid' set in the config. Open to disagreement though.

Also, needs tests added.

Copy link

codecov bot commented Jan 15, 2024

Codecov Report

Merging #1163 (2c3a04c) into main (d7cf72d) will increase coverage by 89.97%.
Report is 2 commits behind head on main.
The diff coverage is 98.83%.

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     
Files Coverage Δ
python/pydantic_core/core_schema.py 94.76% <100.00%> (ø)
src/recursion_guard.rs 100.00% <100.00%> (ø)
src/serializers/extra.rs 99.07% <100.00%> (ø)
src/validators/dataclass.rs 97.80% <100.00%> (ø)
src/validators/definitions.rs 94.80% <75.00%> (ø)

... and 101 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1cb0eb...2c3a04c. Read the comment docs.

Copy link

codspeed-hq bot commented Jan 15, 2024

CodSpeed Performance Report

Merging #1163 will improve performances by 20.58%

Comparing support-dataclass-init (2c3a04c) with main (e1cb0eb)

Summary

⚡ 1 improvements
✅ 142 untouched benchmarks

🆕 3 new benchmarks

Benchmarks breakdown

Benchmark main support-dataclass-init Change
test_core_str 38.1 µs 31.6 µs +20.58%
🆕 test_dataclass_serialization_python N/A 43.2 µs N/A
🆕 test_dataclass_serialization_json N/A 45 µs N/A
🆕 test_dataclass_to_json N/A 71.8 µs N/A

@sydney-runkle sydney-runkle marked this pull request as ready for review January 16, 2024 15:56
Copy link
Collaborator Author

@dmontagu dmontagu left a 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).

Copy link
Contributor

@davidhewitt davidhewitt left a 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 😂

sydney-runkle and others added 2 commits January 17, 2024 06:16
@sydney-runkle
Copy link
Contributor

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.

@davidhewitt,

We could disallow this at schema build time, like I've done here for the extra='allow' and init=False combo: pydantic/pydantic@bb15bac

@davidhewitt
Copy link
Contributor

For now disallowing seems good, we can always relax that in future without breaking backwards compatibility.

@sydney-runkle sydney-runkle merged commit 29c5419 into main Jan 17, 2024
@sydney-runkle sydney-runkle deleted the support-dataclass-init branch January 17, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants