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

Add type hints to builtin models' fields #2214

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

flaeppe
Copy link
Member

@flaeppe flaeppe commented Jun 9, 2024

There are quite a bit of changes in here, but it shouldn't produce any different results really.

It mainly allows pyright to also pick up types from the builtin models.

One note about the types for the fields: they have been copied from what has been declared in _pyi_private_set_type and _pyi_private_get_type for each field, so there shouldn't be any difference for mypy.

Related issues

@Viicos
Copy link
Contributor

Viicos commented Jun 10, 2024

Hopefully this won't play too bad with #1900

@flaeppe
Copy link
Member Author

flaeppe commented Jun 10, 2024

I'm hoping it should play along since #1900 is more about inferring and defaults?

This instead sets explicit and specific get/set types

@intgr
Copy link
Collaborator

intgr commented Jun 16, 2024

This is quite ugly.

Something that I have been pondering about since mypy 1.10 implemented PEP 696 Type Defaults for Type Parameters is: we could add default= to the TypeVars used for model fields.

The downside is that we would need separate TypeVar definitions for most field classes.

# before
class IntegerField(Field[_ST, _GT]):
    _pyi_private_set_type: float | int | str | Combinable
    _pyi_private_get_type: int
    _pyi_lookup_exact_type: str | int

# after
_ST_IntegerField = TypeVar('_ST_IntegerField', default=float | int | str | Combinable)
_GT_IntegerField = TypeVar('_GT_IntegerField', default=int)

class IntegerField(Field[_ST_IntegerField, _GT_IntegerField]):
    _pyi_private_set_type: float | int | str | Combinable
    _pyi_private_get_type: int
    _pyi_lookup_exact_type: str | int

This should improve experience in general for users who don't have the mypy django plugin enabled, and would remove the need for these manual generic arguments:

    password: models.CharField[str | int | Combinable, str]
    last_login: models.DateTimeField[str | datetime | date | Combinable, datetime | None]
    is_active: bool | models.BooleanField[bool | Combinable, bool]

@flaeppe
Copy link
Member Author

flaeppe commented Jun 17, 2024

I don't mind the generic arguments per se, model fields are generic. And I find it great that we can declare them explicitly for models included in the stubs, because then it becomes very clear what types are allowed.

What I do find off is that e.g. CharField declares int as one of its set types. I know that CharField casts/coerces to a string but to me that looks a bit like support for shotgun parsing.

I wouldn't mind a setup that's using default= either. I looked at that initially but was unsure if we would run into any problems until python/mypy#14851 (comment) has been fixed. I'm happy to change to a default= approach if we don't see any troubles with it.

Comment on lines +661 to +681
if self.model_classdef.fullname == fullnames.USER_MODEL_FULLNAME:
permission_typeinfo = self.lookup_typeinfo_or_incomplete_defn_error(fullnames.PERMISSION_MODEL_FULLNAME)
self.create_through_table_class(
field_name="user_permissions",
model_name="User_user_permissions",
model_fullname="django.contrib.auth.models.User_user_permissions",
m2m_args=M2MArguments(
to=M2MTo(arg=self.model_classdef, model=Instance(permission_typeinfo, []), self=False), through=None
),
)
group_typeinfo = self.lookup_typeinfo_or_incomplete_defn_error(fullnames.GROUP_MODEL_FULLNAME)
self.create_through_table_class(
field_name="groups",
model_name="User_groups",
model_fullname="django.contrib.auth.models.User_groups",
m2m_args=M2MArguments(
to=M2MTo(arg=self.model_classdef, model=Instance(group_typeinfo, []), self=False),
through=None,
),
)
return
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I don't think we need these through models in the plugin. I think we can add them explicitly in django/contrib/auth/models.py with a @type_check_only decorator

And then also add the following:

class User(AbstractUser):
    id: models.AutoField[str | int | Combinable | None, int]
    pk: models.AutoField[str | int | Combinable | None, int]
+   groups: models.ManyToManyField[Group, User_groups]
+   user_permissions: models.ManyToManyField[Permission, User_user_permissions]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants