Skip to content

Conversation

adriangb
Copy link
Member

Currently, ModelField.outer_type_ is used for Model.__signature__.

This leads to signatures that differ from dataclasses or classes with a manual __init__ of the same types.

from dataclasses import dataclass
from inspect import signature
from typing import Optional

from pydantic import BaseModel

@dataclass
class Dataclass:
    foo: Optional[int] = None

class CustomClass:
    def __init__(self, foo: Optional[int] = None) -> None:
        ...

class PydanticModel(BaseModel):
    foo: Optional[int] = None


print(signature(Dataclass).parameters["foo"].annotation)  # Optional[int]
print(signature(CustomClass).parameters["foo"].annotation)  # Optional[int]
print(signature(PydanticModel).parameters["foo"].annotation)  # <class 'int'>

This PR fixes this by storing the annotation in ModelField.annotation and then using that for the signature instead of outer_type_.
I realize this is adding a public attribute to ModelField, and that it is not as simple as storing the type_ passed into the constructor (I tried to handle the case where the type is inferred from the default value, I think successfully), so this may not be the best solution, but it is the best I could come up with today.

@adriangb adriangb changed the title Add annotation to ModelField for use in BaseModel.__signature__ bug: Add annotation to ModelField for use in BaseModel.__signature__ Nov 14, 2021
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.

Seems good to me, but please add a change description.

I don't think this can go in v1.9.1 as it's not a bug fix for v1.9

@samuelcolvin
Copy link
Member

please update.

@github-actions github-actions bot added the awaiting author revision awaiting changes from the PR author label Apr 2, 2022
@adriangb
Copy link
Member Author

adriangb commented Apr 2, 2022

Hmm. While I am adding a public attribute, and that could be considered a feature, I am only doing it to fix what I would consider a bug (incorrect results from inspect.signsture). One of my main questions here was: can you think of a way of doing this that does not add a public attribute?

@samuelcolvin
Copy link
Member

I don't mind the public attribute, I don't see another (simply) way.

Which of my points were you questioning? The current description, out which release this goes in?

@adriangb
Copy link
Member Author

adriangb commented Apr 2, 2022

I was just questioning if this is a bug fix or a feature. I think the underlying problem is a bug, but if we're adding a public attribute it's more like a feature.

I'll add the change description.

@adriangb adriangb requested a review from samuelcolvin April 2, 2022 17:43
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

Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
@adriangb
Copy link
Member Author

adriangb commented May 6, 2022

@samuelcolvin I guess we didn't merge this 😅

@samuelcolvin samuelcolvin enabled auto-merge (squash) August 5, 2022 13:48
@samuelcolvin
Copy link
Member

@samuelcolvin I guess we didn't merge this sweat_smile

Got there in the end 😓.

Thank so much, as ever, sorry for the delay.

@JacobHayes
Copy link
Contributor

@samuelcolvin looks like this one won't auto-merge because of the old "request changes" - I think it'll need an approval to override.

@samuelcolvin samuelcolvin merged commit a26665a into pydantic:master Aug 5, 2022
@samuelcolvin
Copy link
Member

Thanks @JacobHayes, missed that.

@JacobHayes
Copy link
Contributor

Yup, I missed the auto-merge announcement so excited to find it in the wild!

@samuelcolvin
Copy link
Member

Ye, it's been a god sent for this work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting author revision awaiting changes from the PR author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants