Skip to content

bug: Add annotation to ModelField for use in BaseModel.__signature__#3413

Merged
samuelcolvin merged 9 commits intopydantic:masterfrom
adriangb:fix-annotated-optional
Aug 5, 2022
Merged

bug: Add annotation to ModelField for use in BaseModel.__signature__#3413
samuelcolvin merged 9 commits intopydantic:masterfrom
adriangb:fix-annotated-optional

Conversation

@adriangb
Copy link
Copy Markdown
Member

@adriangb adriangb commented Nov 14, 2021

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
samuelcolvin
samuelcolvin requested changes Apr 2, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

samuelcolvin commented Apr 2, 2022

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
Copy Markdown
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
Copy Markdown
Member

samuelcolvin commented Apr 2, 2022

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
Copy Markdown
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
samuelcolvin
samuelcolvin reviewed Apr 2, 2022
View reviewed changes
Copy link
Copy Markdown
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

Comment thread changes/3413-fix-inspect-signature.md Outdated
@adriangb @samuelcolvin
Update changes/3413-fix-inspect-signature.md
f667e89 
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
@adriangb
Copy link
Copy Markdown
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
Copy Markdown
Member

samuelcolvin commented Aug 5, 2022

@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
Copy Markdown
Contributor

JacobHayes commented Aug 5, 2022

@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
Copy Markdown
Member

samuelcolvin commented Aug 5, 2022

Thanks @JacobHayes, missed that.

@JacobHayes
Copy link
Copy Markdown
Contributor

JacobHayes commented Aug 5, 2022

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

@samuelcolvin
Copy link
Copy Markdown
Member

samuelcolvin commented Aug 5, 2022

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

Reviewers

@samuelcolvin samuelcolvin samuelcolvin approved these changes

Assignees

@adriangb adriangb

Labels

awaiting author revision awaiting changes from the PR author

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@adriangb @samuelcolvin @JacobHayes