Upgrade django-stubs again, unpin mypy, and add more annotations #36072
Conversation
kdmccormick
force-pushed
the
kdmccormick/unpin-mypy
branch
from
669c6a9 to
3d99ebb
Compare
kdmccormick
changed the title
kdmccormick
changed the title
kdmccormick
changed the title
kdmccormick
force-pushed
the
kdmccormick/unpin-mypy
branch
from
241c7c9 to
b4407a1
Compare
|
@bradenmacdonald or @regisb , I could use one of your pairs of eyes on this, particularly for the new fancy stuff under |
This is possible now that django-stubs and djangorestframework-stubs are unpinned: openedx@a5b773c which became possible once we upgraded to django 4.2. Closes: openedx#35667
kdmccormick
force-pushed
the
kdmccormick/unpin-mypy
branch
from
b4407a1 to
ea34e26
Compare
There was a problem hiding this comment.
This looks great to me. I don't see any issue in openedx.core.types.
I recently discovered that you had added mypy testing to edx-platform CI, which is great! Coverage is still lacking of course, but I'm hoping we can improve that in the future. Maybe with a hackathon?
I also love how your PR is fixing actual bugs that you discovered thanks to mypy.
Rather than constraining django-stubs' major version to our django major version (4.x.x), we are going to go one ahead (5.x.x), as recommended by python/mypy#17958 Also includes an unrelated common_constraints update.
Includes some new Request type annotations in openedx.core.types.http, plus a new meta-utility @type_annotation_only to ensure that we don't accidentally start instantiating those new classes.
kdmccormick
force-pushed
the
kdmccormick/unpin-mypy
branch
from
ea34e26 to
69bcbac
Compare
You can thank Braden for getting the ball rolling with mypy in this repo! I too would love to increase coverage--if you kicked off a hackathon, I would certainly throw my weight behind it. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
The very first work on mypy in platform was done by @regisb, via #27575 + #27786 as far as I know! I was just the first to build on it and expand to multiple apps in this repo (#32591). Nice work on this PR @kdmccormick :) |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
|
Oh right--Regis added mypy, OC has been using it a lot, and I think Axim/Arbisoft added it to CI.
Regardless, good work all around--types are great and I want to use them everywhere :) |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
This gets us to the latest version of both django-stubs and mypy, while constraining django-stubs<6, which should ensure that the stubs stay compatible as long as we are on django 4.x.x or django 5.x.x. We had previously upgraded django-stubs from 1.16.0 to 4.2.7, but it seems that, due to a regression in django-stubs 4.2.7, we should just go all they way forward to django-stubs 5.x.x.
This will let us take advantage of mypy's latest features and bugfixes, allowing us to write richer type annotations. It will also help ensure that any type-checked modules are more likely to be compatible with django 5+. It should have no functional impact.
As part of this, we've needed to introduce some new utilities in the openedx.core.types package.
See commits for more details.
Closes: #35667