Skip to content

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
davidhewitt pushed a commit to pydantic/pydantic that referenced this pull request Oct 20, 2025
Co-authored-by: sydney-runkle <[email protected]>
Co-authored-by: Sydney Runkle <[email protected]>
Co-authored-by: David Hewitt <[email protected]>

Original-commit-hash: 29c5419
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