search

sbdchd / django-types

:doughnut: Type stubs for Django
MIT License
188 stars 62 forks source link

Remove the overloads for choices and simplify typing #87

Closed bellini666 closed 2 years ago

bellini666 commented 2 years ago

Hey @sbdchd ,

I'm thinking a lot regarding the change I've proposed. Although it is very nice, it complicated the typing of the fields a lot and it has some problems with pyright and mypy due to the fact that they require both __init__ and __new__ to be overloaded.

I'm thinking in opening a PR simplifying that and removing the __init__ from everything. I think it should be enough to define just 2 __new__ (one for the null=True other for the null=False) with the accepted arguments.

Do you agree with my point of view here?

last-partizan commented 2 years ago

Oh, i think this is related to #85

And looks like removing overload for __init__ will fix it.

last-partizan commented 2 years ago

I've raised issue in pyright repo regarding this issue.

And looks like thos overrides was using undocumented typing feature (only available in mypy).

https://github.com/microsoft/pyright/issues/2909#issuecomment-1019374993

I've already created PR removing those oveloads: https://github.com/sbdchd/django-types/pull/86

But, it breaks mypy, for some reason? does it not support overloading __new__? or did i missed something?

@sbdchd @bellini666 please take a look at PR, maybe we're figure out how to fix it.

Or, maybe this is issue with mypy and we should create bug report there?

bellini666 commented 2 years ago

Hey @last-partizan ,

I commented at your issue and I think it is something different. The point of this issue is mostly a simplification of the complexity that I created a while ago when I tried to capture the choices option to make, for example, a CharField(choices=[("foo", "Foo"), ("bar", "Bar)) be typed as CharField[Literal["foo", "bar"]] instead of CharField[str].

It works, but it created a very large complexity in the stubs that I don't know if it paid off. Also in the end I ended up creating this (https://github.com/bellini666/django-choices-field), to have choices properly mapped, which is probably the way to go when someone requires that much specificity for the overload.

bellini666 commented 2 years ago

Damn, I wrote an awesome explanation about this issue in this comment. At the end of it it is very clear why this proposed change is a good change.