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: support arbitrary types with custom __eq__ and Annotated with python 3.9 #2502

Merged
merged 1 commit into from May 9, 2021

Conversation

PrettyWood
Copy link
Member

@PrettyWood PrettyWood commented Mar 9, 2021

Change Summary

When the default value is an instance of a class with a custom __eq__, it can fail as shown in the written test.
But the question is "why did we switch from an identity check against Undefined to an equality check against Undefined AND Ellipsis?`
The answer lies here
https://github.com/samuelcolvin/pydantic/blob/62bb2ad4921016df51abf3922c3fe51113b08939/tests/test_annotated.py#L121-L130

To support this, a check on Ellipsis has been added, which is wrong IMO.
It has been done because the default value of the Field is not the same once it has been validated (Undefined becomes Required aka Ellipsis)

2 solutions:

  • store also the default value of the field in original_default and use this one for the check against Undefined
  • do not change the default value and keep Undefined

I went with solution 2 as it doesn't seem to have any impact

Related issue number

closes #2483
closes #2639

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 Mar 9, 2021

Codecov Report

Merging #2502 (bdd0ea8) into master (7b7e705) will not change coverage.
The diff coverage is 100.00%.

❗ Current head bdd0ea8 differs from pull request most recent head 683b0c1. Consider uploading reports for the commit 683b0c1 to get more accurate results

@@            Coverage Diff            @@
##            master     #2502   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         5109      5108    -1     
  Branches      1050      1050           
=========================================
- Hits          5109      5108    -1     
Impacted Files Coverage Δ
pydantic/decorator.py 100.00% <100.00%> (ø)
pydantic/fields.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 14f055e...683b0c1. Read the comment docs.

@PrettyWood PrettyWood added ready for review bug V1 Bug related to Pydantic V1.X labels Mar 15, 2021
@PrettyWood PrettyWood added this to the v1.8.2 milestone Mar 30, 2021
@PrettyWood PrettyWood changed the title fix: support arbitrary types with custom __eq__ fix: support arbitrary types with custom __eq__ and Annotated with python 3.9 Apr 6, 2021
@samuelcolvin
Copy link
Member

awesome, thank you so much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug V1 Bug related to Pydantic V1.X ready for review
Projects
None yet
2 participants