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

Upgrade to Mypy 1.0 #339

Merged
merged 5 commits into from
Feb 21, 2023
Merged

Conversation

christianbundy
Copy link
Contributor

@christianbundy christianbundy commented Feb 6, 2023

I have made things!

Upgrade to Mypy 1.0

Related issues

Refs python/mypy#13685

Closes #340

@intgr
Copy link
Contributor

intgr commented Feb 6, 2023

Approved CI run.

@intgr
Copy link
Contributor

intgr commented Feb 6, 2023

Some if not all of the typecheck errors are due to upstream changes in DRF test suite, not from upgrading mypy.

@christianbundy
Copy link
Contributor Author

Do we just add them as excluded errors in ./scripts/typecheck_tests.py? Trying to make sure I understand the procedure.

@intgr
Copy link
Contributor

intgr commented Feb 7, 2023

Do we just add them as excluded errors in ./scripts/typecheck_tests.py?

I've looked at each error, and checked if it's something we're missing in stubs or a false positive. Usually it's a false positive, or some new feature that has been merged in DRF but will be in the next version (e.g. #338).

But it's annoying and confusing, especially for new contributors, when CI checks start to fail due to unrelated changes. So I've been thinking that we may want to freeze the git commit hash that we check (like is already done in django-stubs).

But the downside is that then we would have to periodically update this hash, something that's been a bit neglected in django-stubs.

@intgr intgr force-pushed the christian/mypy-1.0 branch from 13ba668 to 36c5e8a Compare February 9, 2023 09:30
@intgr
Copy link
Contributor

intgr commented Feb 9, 2023

I rebased your branch. The remaining CI errors are from mypy 1.0 update.

@christianbundy
Copy link
Contributor Author

Thanks! For convenience, here are the relevant files:

Problem 1

drf_source/tests/test_permissions.py:593: error: "<typing special form>" not callable
drf_source/tests/test_permissions.py:599: error: "<typing special form>" not callable
drf_source/tests/test_permissions.py:653: error: "<typing special form>" not callable
drf_source/tests/test_permissions.py:661: error: <nothing> not callable
drf_source/tests/test_permissions.py:673: error: "<typing special form>" not callable
drf_source/tests/test_permissions.py:681: error: <nothing> not callable
drf_source/tests/test_permissions.py:736: error: "<typing special form>" not callable

These seem to be exclusively complaining about this pattern:

composed_perm = permissions.IsAuthenticated | permissions.AllowAny
assert composed_perm().has_permission(request, None) is True

This only happens when you __or__ permissions, and it looks like __and__ is unaffected.

I don't really understand why this would happen, since the stub definition seems to treat | and & similarly, but I'm inclined to think that maybe this is a bug in Mypy where | is being treated as a type union. What do you think?

Problem 2

drf_source/tests/test_versioning.py:28: error: "None" not callable
drf_source/tests/test_versioning.py:35: error: "None" not callable
drf_source/tests/test_versioning.py:43: error: "None" not callable
drf_source/tests/test_versioning.py:50: error: "None" not callable

These are all coming from:

self.versioning_class()

Where self is a subclass of APIView with a get() method defined.

The stub defines this as nullable, and DRF source seems to agree that this is possible, so I can add an ignore for this one.

@intgr
Copy link
Contributor

intgr commented Feb 9, 2023

I don't really understand why this would happen, since the stub definition seems to treat | and & similarly, but I'm inclined to think that maybe this is a bug in Mypy where | is being treated as a type union. What do you think?

Yes indeed, mypy thinks composed_perm is a type alias with Union. For now, an ignore is fine.

I always hated how DRF requires permissions checks to be classes, not instances.

This can cause problems in actual user code. Maybe report a mypy bug? Mypy could look at whether a class overrides the __or__ method, or look at the method's return value, to detect this. (Does it matter that this comes from the metaclass?)

@christianbundy christianbundy marked this pull request as ready for review February 18, 2023 00:21
Copy link
Contributor

@intgr intgr left a comment

Choose a reason for hiding this comment

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

There are still some test failures. Sorry, I could have approved the CI run earlier.

scripts/typecheck_tests.py Outdated Show resolved Hide resolved
@christianbundy
Copy link
Contributor Author

Done! And I ran act locally and it looks like it passed all tests. (Though I'd love to be able to run CI without approvals if that's easy to fix.)

@flaeppe
Copy link
Member

flaeppe commented Feb 21, 2023

Done! And I ran act locally and it looks like it passed all tests. (Though I'd love to be able to run CI without approvals if that's easy to fix.)

I've approved the CI run. You shouldn't need approvals once you've become a contributor.

Copy link
Contributor

@intgr intgr left a comment

Choose a reason for hiding this comment

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

Thanks!

@intgr intgr merged commit 1e7cfe9 into typeddjango:master Feb 21, 2023
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