search

sbdchd / django-types

:doughnut: Type stubs for Django
MIT License
202 stars 63 forks source link

Improve and simplify field typing overloads #97

Closed bellini666 closed 2 years ago

bellini666 commented 2 years ago

Fix #87 Fix #85

bellini666 commented 2 years ago

@sbdchd just like we talked, this sets __new__ overload on all fields instead of using a mix of __init__/__new__, which is not a documented behaviour.

bellini666 commented 2 years ago

Hey @sbdchd , is everything fine with this PR?

sbdchd commented 2 years ago

Sorry for the delay! I'm always super hesitant with big changes because I want to avoid breaking downstream consumers.

Can we add some tests to show what we get from this change?

bellini666 commented 2 years ago

@sbdchd np, I understand your hesitation! :)

The change that I'm removing is the one that I previously forced and even ended up not even using it because I prefer to use my https://github.com/bellini666/django-choices-field to properly type the field with enums.

I know see that I overcomplicated the typing, which makes maintaining and improving it a lot harder, and also produced issues with the fact that we have both a __init__ and a __new__ (the alternative was to have 4 __new__ for literally each field, which is also really bad in terms of maintenance)

Other than that, everything should be literally the same.

obs. If someone still wants to type the field's choices, he can do something like:

class SomeModel(models.Model):
    choices = Literal["foo", "bar", "bin"]
    some_field = models.CharField[choices]()

(but I highly doubt that anyone was depending on this)

Can we add some tests to show what we get from this change?

Hrm, I could do that, but don't know actually what to write for tests because we are not getting anything from this change other than a very nice and needed simplification of the fields' overloads.

It fixes the #85, but it is actually a side effect of the fact that we are not doing the wrong thing of having both __init__ (with self being typed) and __new__ at the same classes.

sbdchd commented 2 years ago

Thank you for the explanation and all your hard work, looks great!