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

Preserve annotations from bases #758

Merged

Conversation

dmontagu
Copy link
Contributor

@dmontagu dmontagu commented Aug 17, 2019

Change Summary

Preserve annotations from superclasses when modified in subclasses, even if the annotation is not repeated. (This prevents the field from being inferred as a different type.)

Related issue number

Fixes #757

Checklist

@samuelcolvin I don't think this change affects any existing documentation, and it feels subtle enough to me that it probably doesn't merit new docs (I think people should have assumed things would work this way, like @kszucs did). But let me know if you'd prefer I add a note.

  • 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>.rst file added describing change
    (see changes/README.md for details)

@dmontagu dmontagu force-pushed the preserve-superclass-annotations branch from e306fc7 to 6553ee3 Compare August 17, 2019 08:16
@codecov
Copy link

codecov bot commented Aug 17, 2019

Codecov Report

Merging #758 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #758   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines        2726   2726           
  Branches      537    536    -1     
=====================================
  Hits         2726   2726

@kszucs
Copy link

kszucs commented Aug 17, 2019

@dmontagu for my PR the following snippet has failed with validation error for the list field:

from pydantic import BaseModel
from typing import List


class Base(BaseModel):
        i: int = 1
        s: str = 'string'
        f: float = 1.1
        l: List[str] = []


class Child(Base):
    s = 'str'
    l = ['a']

Child(i=4, l=['a', 'b'])

I think this solution is more robust so I'm closing mine in favour of this PR.

@kszucs kszucs mentioned this pull request Aug 17, 2019
4 tasks
value: int = 1

class B(A):
value = 'G'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens in the case that I change this to value: str = 'G'?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should update the type annotation to str, I guess a test case is required.

tests/test_edge_cases.py Outdated Show resolved Hide resolved
@dmontagu dmontagu force-pushed the preserve-superclass-annotations branch from 4ed8b86 to 2e2e9e1 Compare August 17, 2019 21:02
Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise LGTM.

tests/test_edge_cases.py Outdated Show resolved Hide resolved
tests/test_edge_cases.py Show resolved Hide resolved
tests/test_edge_cases.py Show resolved Hide resolved
@kszucs
Copy link

kszucs commented Aug 20, 2019

@dmontagu @samuelcolvin many thanks for the fix!

@samuelcolvin samuelcolvin merged commit 5e8db16 into pydantic:master Aug 21, 2019
@kszucs
Copy link

kszucs commented Aug 26, 2019

@dmontagu @samuelcolvin it doesn't work for nested types, example to reproduce:

    class A(BaseModel):
        seq: List[int] = [0, 1, 2]

    class B(A):
        seq = [4, 5]

    class C(A):
        seq = []
Inferred B.seq type=list default=[4, 5]
A.seq type=int default=[0, 1, 2]

@dmontagu
Copy link
Contributor Author

dmontagu commented Aug 26, 2019

@kszucs It looks like maybe the example you shared wasn't run against the current master branch (note: master is targeting 1.0, rather than 0.32.x, I think). On master, your example actually breaks:

from typing import List

from pydantic import BaseModel


class A(BaseModel):
    seq: List[int] = [0, 1, 2]


class B(A):
    seq = [4, 5]

# TypeError: The type of B.seq differs from the new default value; if you wish to change the type of this field, please use a type annotation

This is because we currently don't infer value types. If you reannotate the field as List[int] in the subclass, it just works.

If you find this annoying and would prefer it behave differently I think that could make sense -- in that case, could you create a new issue for discussion?

(At the very least, the error message should probably be a little clearer about how the inferred type differed; I think this specific example would be a confusing error message to deal with.)

@kszucs
Copy link

kszucs commented Aug 26, 2019

@dmontagu yep, I would expect it not to break. Sure I can create a new issue.

alexdrydew pushed a commit to alexdrydew/pydantic that referenced this pull request Dec 23, 2023
Co-authored-by: Samuel Colvin <s@muelcolvin.com>
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.

Overriding default value makes __fields__ inconsistent with __annotations__
3 participants