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

Declaring field types as sub-classes of base types gets highlighted in #312

Closed
tuukkamustonen opened this issue May 31, 2021 · 8 comments
Closed

Comments

@tuukkamustonen
Copy link

tuukkamustonen commented May 31, 2021

Hi, thanks for the great plugin!

Describe the bug

When using sub-classed Constrained* fields, Expected type '...', got '...' instead. error is shown.

See this example (using StrictStr which is inherited from ConstrainedStr):

image

It reproduces even when subclassing from base types, ie. str:

import pydantic

class MyStr(str):
    pass

class MyModel(pydantic.BaseModel):
    field: MyStr

MyModel(field='')

To Reproduce

Above.

Expected behavior

There's shouldn't be error.

Screenshots

Uh, above.

Environments (please complete the following information):

  • IDE: PyCharm Professional 2021.1
  • OS: Kubuntu 20.04 LTS
  • Pydantic Version: 1.8.2
  • Plugin version: 0.3.3
@tuukkamustonen
Copy link
Author

What is unfortunate here, is that declaring fields as sub-classes (above) is the only to avoid mypy nag about them. If you use constr() et al. mypy will report error.

@koxudaxi
Copy link
Owner

@tuukkamustonen

The problem is not a bug.
You can disable or change the warning.
Please check these documents.

https://koxudaxi.github.io/pydantic-pycharm-plugin/type-checker-for-pydantic/

I put an example for working your code.

project structure

.
├── your_app
│   ├── __init__.py
│   └── main.py
└── pyproject.toml

pyproject.toml

[tool.pydantic-pycharm-plugin.parsable-types]

# str field may parse int and float
str = ["int", "float"]

# datetime.datetime field may parse int
"datetime.datetime" = [ "int" ]

# your_module.your_type field may parse str
"your_app.main.MyStr" = [ "str" ]

[tool.pydantic-pycharm-plugin]
# You can set higlith level (default is "warning")
# You can select it from "warning",  "weak_warning", "disable"
parsable-type-highlight = "warning"

## If you set acceptable-type-highlight then, you have to set it at same depth.
acceptable-type-highlight = "disable"

Screenshot from 2021-06-01 01-15-22

@tuukkamustonen
Copy link
Author

Uh, you're completely right (how else!). I did miss that bit in the docs. I just didn't expect that you have to configure the plugin for a basic use case.

After adding pyproject.toml and tuning it properly, it works perfectly. Thanks for the guidance!

It is kinda cumbersome having to maintain these mappings in a configuration file, don't you think? Have you considered a mode, where any str based type if accepted as any str based type, any int based type could be accepted as any int based type, and so on? (I'm not familiar with PyCharm introspection - zero idea what can or can not be done.)

@tuukkamustonen
Copy link
Author

I found something, though. Even if I whitelist Str1 below in pyproject.toml, wrapping it inside a Dict and then Optional gets false positive warning:

from typing import Dict, Optional

from pydantic import BaseModel, StrictStr


class Str1(StrictStr):
    pass


class InnerModel(BaseModel):
    pass


class Model(BaseModel):
    field1: Str1
    field1_opt: Optional[Str1]
    field2: Dict[str, int]
    field2_opt: Optional[Dict[str, int]]
    field3: Dict[Str1, int]
    field3_opt: Optional[Dict[Str1, int]]
    field4: Dict[Str1, InnerModel]
    field4_opt: Optional[Dict[Str1, InnerModel]]


Model(
    field1='fo',
    field1_opt='fo',
    field2={'bar': 2},
    field2_opt={'bar': 2},
    field3={'bar': 2},
    field3_opt={'bar': 2},  # incorrectly highlighted
    field4={'bar': InnerModel()},
    field4_opt={'bar': InnerModel()},  # incorrectly highlighted
)

image

Should I open another ticket about it?

@koxudaxi
Copy link
Owner

koxudaxi commented Jun 1, 2021

any int based type could be accepted as any int based type, and so on?

That looks great idea.
type-checker would allow inherited-type with a new option.
The option will accept include/exclude inherited types.
What do you think about it?

I found something, though. Even if I whitelist Str1 below in pyproject.toml, wrapping it inside a Dict and then Optional gets false positive warning:

I guess it may a bug.
I will check the code.

@tuukkamustonen
Copy link
Author

tuukkamustonen commented Jun 1, 2021

type-checker would allow inherited-type with a new option.

Yes, that would be a great start. But it should also:

class MyStr1(StrictStr): pass
class MyStr2(StrictStr): pass

...accept either of above as another, although they don't inherit each other.

Considering that pydantic already maps the constrained types as base types when running type checking, simply doing the same with your plugin might be a viable option.

So, if it could always resolve "base type" (e.g. str, int, etc.) then that could just do it.

I believe that is something like what will happen in pydantic v2 anyway:

class Pokemon(BaseModel):
    name: Annotated[str, Constraints(min_length=1)] = Field('pikachu', alias='Name')

Here, you can just pick the str as the type, because there are no inherited types anymore (validation gets moved to separate classes).

I guess it may a bug.
I will check the code.

👍🏻

@koxudaxi
Copy link
Owner

koxudaxi commented Jun 1, 2021

Considering that pydantic already maps the constrained types as base types when running type checking, simply doing the same with your plugin might be a viable option.

Some users may want to check strict types strictly. 🤔
This option will cover all case.

[tool.pydantic-pycharm-plugin]
constrained-type = "strict"  # or "lenient"

This plugin already has supported Annotated 😉

@tuukkamustonen
Copy link
Author

tuukkamustonen commented Jun 1, 2021

Some users may want to check strict types strictly.

Yeah, that's true and why not. It just gets tricky quickly and simply staying on top of base types is something that should work sufficiently without extra magic 🌟

So, I think constrained-type = "lenient" mode should be a good workaround/escape mechanism when all else fails 😄

This plugin already has supported Annotated

That's great. What I mean is that with Annotated we don't need this constrained-type = "lenient" logic at all, because with Annotated you always use the base type as the first argument, like:

class Model(BaseModel):
    field: Annotated[str, Field(min_length=1)]

Constrained* classes and con*() methods are not supported with Annotated, so there are no sub-types to worry about. But that also means that strict types are not supported now with Annotated 😢 (as there's no Field(strict=...)) so it's sensible to support also non-Annotated declarations, like you're about to do here.

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

No branches or pull requests

2 participants