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

mypy plugin #460

Closed
canassa opened this issue Apr 4, 2019 · 9 comments · Fixed by #722
Closed

mypy plugin #460

canassa opened this issue Apr 4, 2019 · 9 comments · Fixed by #722
Labels
feature request help wanted Pull Request welcome

Comments

@canassa
Copy link

canassa commented Apr 4, 2019

Feature Request

I believe that this project could benefit from having a small mypy plugin built for it. Errors like #400 or #280 could be addressed in this plugin.

References

@samuelcolvin
Copy link
Member

great idea, yes please.

I won't have time to work on it for a while but happy to review a PR if anyone else is keen.

@dmontagu
Copy link
Contributor

dmontagu commented Aug 6, 2019

@samuelcolvin I've implemented an initial relatively-limited-in-scope mypy plugin.

https://gist.github.com/dmontagu/381d03126c3b35d58274798a19fc12fa
(Edit: See #722 for the most up-to-date version)

Currently, all it does is replace the signature of the __init__ of subclasses of BaseModel using the annotations so that it can type check it properly. Currently it disallows any unexpected field names, and probably doesn't properly handle non-annotated fields.

(There is a subtlety here in that you may want to be able to explicitly pass kwargs to the model in your code that it doesn't expect (e.g., if using a Union type), but this bites me much more frequently than it is useful. I think it should probably be a config setting. It's probably possible to make use of the Config in the plugin (e.g., to read off the extra property) with a little effort.)

Having gotten this far, I think it wouldn't be too hard to implement more features and/or more accurate edge-case handling, but it would be nice if we could put together a list of desired features.

The __init__ checking was the highest priority for me, but I think having proper type-checking for the result of create_model would be nice (and not too hard, actually; it's very similar to what is done in the sqlalchemy plugin for declarative_base I think). I'm sure there are other things I'm also not thinking of.

@dmontagu
Copy link
Contributor

dmontagu commented Aug 7, 2019

Current state of the plugin (in the above gist):
(Edit: See #722 for the most up-to-date version)

from pydantic import BaseModel, Extra, Schema


class A(BaseModel):
    a: int
    b: int = Schema(...)  # type: ignore
    c: int = Schema(None)  # type: ignore
    d: int = 1

    class Config:
        allow_mutation = True
        extra = Extra.forbid


a_instance = A(a=1, b=1, c=1, d=1, e=1)
# error: Unexpected keyword argument "e" for "A"

a_instance = A(c="")
# error: Missing named argument "a" for "A"
# error: Missing named argument "b" for "A"
# error: Argument "c" to "A" has incompatible type "str"; expected "int"

a_instance.a = 2
# (No errors)


class B(A):
    class Config:
        allow_mutation = False
        extra = Extra.allow


b_instance = B(a=1, b=1, c=1, d=1, e=1)
# (No errors)

b_instance = B(c="")
# error: Missing named argument "a" for "B"
# error: Missing named argument "b" for "B"
# Argument "c" to "B" has incompatible type "str"; expected "int"

b_instance.a = 2
# error: Property "a" defined in "B" is read-only

The only things I can think of that remain unhandled are 1) handling generics/create_model, 2) not causing conflicts when assigning to Schema, and 3) maybe handling a couple other config settings.

@samuelcolvin
Copy link
Member

Let's start with something small and build further features as we go forward.

Very dumb/simple question: how is this plugin distributed? As part of pydantic or as a separate package?

@dmontagu dmontagu mentioned this issue Aug 7, 2019
4 tasks
@koxudaxi
Copy link
Contributor

koxudaxi commented Aug 9, 2019

@dmontagu
I try your mypy plugin. I found a problematic case. Please check the result.

python source

from datetime import date
from pydantic import BaseModel


class A(BaseModel):
    a: date


A(a=date(2019, 1, 1))
A(a=1000)
A(a='2019-01-01')

output

$ mypy v.py
v.py:10: error: Argument "a" to "A" has incompatible type "int"; expected "date"
v.py:11: error: Argument "a" to "A" has incompatible type "str"; expected "date"

This code is valid for pydantic. However, the type is not valid. pydantic cast some object from another type. How should the mypy plugin assert types?

Would you tell me your thinking about the case?

@dmontagu
Copy link
Contributor

dmontagu commented Aug 9, 2019

I have configured the plugin to do type-checking on the __init__ arguments. This is actually the same way that your awesome PyCharm plugin behaves -- I think you'll see warnings for those cases there as well.

The way I imagine incorporating the plugin into a code base, is that you whenever you initialize a model using named kwargs (rather than **kwargs), mypy would demand that you to ensure the types were correct. This still lets you use the models to perform parsing by initializing with a **kwargs_dict without mypy errors, it just helps you type check the inputs where you provide them.

I think it is probably not feasible / maintainable to design the plugin to understand whether one type will be successfully parsed into another. On the other hand, if there is no type-checking on the __init__ kwargs, I think the plugin loses a lot of value.


It would not be hard to modify the plugin to annotate each variable in the __init__ signature as having type Any; this would allow you to still at least check that you received all necessary kwargs, and no forbidden kwargs. And it wouldn't be hard to make it so that this could be turned on/off by specifying a different plugin name in your mypy config.

But I think this is of substantially lower type-checking value -- for maintenance purposes, I would rather design my code base in such a way that the Model.__init__ calls are always correctly typed (or I use a # type: ignore if not), and if I want to parse I either pass a **kwargs to the Model.__init__ or call parse_obj.

There is always the option to just not use the plugin as well; that will result in the smallest number of false positives 😄.

@koxudaxi Let me know what you think about this.

@dmontagu
Copy link
Contributor

dmontagu commented Aug 9, 2019

Also, @koxudaxi not sure whether you are using the gist version or the PR version of the plugin, but I'll push any updates to the PR version (#722). (I don't think it handles the issue you pointed out any differently, but I'm not sure whether that gist is currently up-to-date, and it will likely not remain up-to-date as I put more effort into the mypy plugin).

@koxudaxi
Copy link
Contributor

@dmontagu

Thank you for your answer.

OK, I make the pycharm-plugin which behave in the same way as the mypy plugin. 🙂
If the plugin is merged into master, then we may need a few examples.

I have already used the PR version.

@dmontagu
Copy link
Contributor

#722 has been fleshed out pretty extensively, and now includes full docs describing its capabilities. I would appreciate if anyone interested in this could take a look and provide comments.

alexdrydew pushed a commit to alexdrydew/pydantic that referenced this issue Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request help wanted Pull Request welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants