Skip to content

Fill remaining missing hints with Any & disallow partial hints #1206

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

Merged

Conversation

intgr
Copy link
Collaborator

@intgr intgr commented Oct 27, 2022

This one just adds Any to all the remaining missing hints (most of those files are unhinted anyway) and disallows partial hints in mypy.ini

disallow_untyped_defs = true
disallow_incomplete_defs = true

@sobolevn
Copy link
Member

@intgr can you please solve merge conflicts? And I can then start reviewing it :)

@intgr intgr force-pushed the fill-remaining-missing-hints-with-Any branch from 62ba65d to 7324e63 Compare October 28, 2022 10:10
@intgr intgr marked this pull request as ready for review October 28, 2022 10:11
# non-Model instances
@overload
def __get__(self: _T, instance, owner) -> _T: ...
def __get__(self: _T, instance: Any, owner: Any) -> _T: ...
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm unsure what these owner fields are.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's type[M] | None = None (if not in pyi)

Where

M = TypeVar("M", bound=Model)

And I think it's not none when instance is not none.

Making this an @overload function

Copy link
Member

Choose a reason for hiding this comment

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

So in this specific overload both any's should be : None I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@flaeppe You're probably right but I'm wary of touching these since I don't totally understand what is happening here. Are you up for opening a follow-up PR?

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be fine keeping it as Any since what's exposed as an API is _T. The instance and owner types would only be relevant for the implementation body, but this is in stubs so we won't be affected here.

The only time they'd come in to play and have an impact would be while/if subclassing Field and overriding it's descriptor methods. If we keep Any, we'll allow a child class to declare them as whatever type they deem fit, while doing e.g. type[M] | None we'll restrain it.

Though, in general I don't see any obvious use cases for overriding the descriptor for Field, but I might be wrong here. The normal Django use case would be to implement a separate field descriptor class (https://docs.djangoproject.com/en/4.1/ref/models/fields/#django.db.models.Field.descriptor_class) to decide what goes in and out of a model field

@intgr
Copy link
Collaborator Author

intgr commented Oct 28, 2022

def __len__(self) -> Any: ...

Technically it is always int

The idea here was to fill in all missing hints so that we could increase the strictness in mypy.ini, to make sure people don't forget to add hints to new arguments or return values in the future.

I'm not making anything worse because Any was implied already before for all these unhinted items.

I suppose I could do manual tweaks here and there, but it's increasing the scope of a change that's already enormous. Is that what we want?

@sobolevn
Copy link
Member

I'm not making anything worse because Any

Ok!

@adamchainz
Copy link
Contributor

I'm okay with this, but after merging, can you open an issue or PR for -> int on all __len__ methods? autotyping can also help with adding some type hints automatically.

@intgr intgr marked this pull request as draft October 30, 2022 22:27
@intgr intgr force-pushed the fill-remaining-missing-hints-with-Any branch 2 times, most recently from e7662d9 to faf8ff2 Compare October 30, 2022 22:41
@sobolevn
Copy link
Member

is it ready to be reviewed? or should we merge tests first?

@intgr
Copy link
Collaborator Author

intgr commented Oct 31, 2022

It contains all changes from the other PR also. But other than that it should be ready.

@sobolevn
Copy link
Member

I've merged test changes, there are conflicts now 👀

@intgr intgr force-pushed the fill-remaining-missing-hints-with-Any branch from faf8ff2 to 81d05ee Compare October 31, 2022 09:34
@intgr
Copy link
Collaborator Author

intgr commented Oct 31, 2022

Rebased.

@intgr intgr marked this pull request as ready for review October 31, 2022 09:36
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks again! Maybe someone else will have some extra input.

Copy link
Contributor

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

LGTM as a step towards stricter Mypy config.

@sobolevn sobolevn merged commit 8f475fa into typeddjango:master Oct 31, 2022
@intgr intgr deleted the fill-remaining-missing-hints-with-Any branch October 4, 2023 23:13
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.

4 participants