Skip to content
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: stdlib dataclass converted into pydantic dataclass still equal its stdlib dataclass equivalent #2182

Closed
wants to merge 2 commits into from

Conversation

PrettyWood
Copy link
Member

@PrettyWood PrettyWood commented Dec 7, 2020

Change Summary

Related issue number

closes #2162

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@codecov
Copy link

codecov bot commented Dec 7, 2020

Codecov Report

Merging #2182 (b177fb4) into master (de0657e) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2182   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines         4121      4125    +4     
  Branches       829       830    +1     
=========================================
+ Hits          4121      4125    +4     
Impacted Files Coverage Δ
pydantic/dataclasses.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

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

@PrettyWood PrettyWood changed the title fix: stdlib dataclass automatically converted into pydantic dataclass can still equal its stdlib dataclass equivalent fix: stdlib dataclass converted into pydantic dataclass still equal its stdlib dataclass equivalent Dec 8, 2020
@samuelcolvin
Copy link
Member

Humm, my only reservation is that standard python dataclasses to clever stuff so that different dataclasses with the same fields (and values) are not equal:

from dataclasses import dataclass

@dataclass(frozen=True)
class Foo:
    a: int
    b: float

@dataclass(frozen=True)
class Bar:
    a: int
    b: float

f = Foo(a=10, b=1.0)
b = Bar(a=10, b=1.0)
debug(f == b)  #> False

f2 = Foo(a=10, b=1.0)
debug(f == f2)  #> True

Isn't there a risk that different dataclasses incorrectly being equal is worse than them incorrectly being unequal?

I know "correct" and "incorrect" are subjective here and some might argue (very reasonably) that f and b are the same above, but I think since dataclasses are a python thing we should do our best to match python behaviour.

If we can't reliably match python's standard behaviour, I'd be happy to just raise an error some people don't assume stuff will work which won't.

@samuelcolvin
Copy link
Member

samuelcolvin commented Jan 1, 2021

I think it shouldn't be that hard to copy python's own logic more or less verabim:

https://github.com/python/cpython/blob/1e5d33e9b9b8631b36f061103a30208b206fd03a/Lib/dataclasses.py#L949-L955

@jtrh
Copy link

jtrh commented Jan 10, 2021

I think it shouldn't be that hard to copy python's own logic more or less verabim:

https://github.com/python/cpython/blob/1e5d33e9b9b8631b36f061103a30208b206fd03a/Lib/dataclasses.py#L949-L955

I agree that copying Python's own logic is a good idea, but how would you handle the class equality check? Read below for details.

Near the end of that code, Python calls _cmp_fn() to create the comparison function, which starts by checking for class equality (if other.__class__ is self.__class__:) before even taking a look at the fields themselves.

@samuelcolvin Do you think that replacing the class equality check with something like if other.__class__ is self.__class__ or other.__class__ is self.__class__.__bases__[0]: would be reliable enough?

@samuelcolvin
Copy link
Member

@samuelcolvin Do you think that replacing the class equality check with something like if other.__class__ is self.__class__ or other.__class__ is self.__class__.__bases__[0]: would be reliable enough?

Yes, I think makes sense.

@PrettyWood
Copy link
Member Author

PrettyWood commented Mar 21, 2021

FYI I thought again about all the issues related to stdlib dataclass to validated dataclass
I started a draft on implementation in #2557.
Feedback more than welcome! 🙏

@PrettyWood
Copy link
Member Author

Will be tackled in #2557

@PrettyWood PrettyWood closed this Mar 27, 2021
@PrettyWood PrettyWood deleted the fix/dataclass-equality branch March 27, 2021 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pydantic dataclasses break __eq__ method generated by stdlib dataclasses when upgrading from v1.6.1 to v1.7.2
3 participants