Django-rest-framework: Merge stubs from djangorestframework-stubs into the codebase

[mkurnikov]

Hi, I'm the primary maintainer of https://github.com/typeddjango/djangorestframework-stubs and https://github.com/typeddjango/django-stubs

As Python 2 support is dropped, would you be open to merging mentioned stubs into the codebase?

I can make a PR with the change, if it will be accepted. I can also help with a mypy plugin to return a proper TypedDict from the serializer.data attribute, based on defined fields
https://www.python.org/dev/peps/pep-0589/

[carltongibson]

Hi. I think we need to double down on the discussion on Django here.

It seems to me that it got stalled some time back, because typing wasn’t great, but that it’s come a long way since then, but the objections haven’t been addressed.

More or less, if we can say Django is going to go for it then big yes to DRF.

How does the discussion seem to you?

[mkurnikov]

It got stuck on me writing a DEP=) Could we maybe iterate on it over email with you? It would be much more comfortable for me than writing it all at once.

As for

It seems to me that it got stalled some time back, because typing wasn’t great, but that it’s come a long way since then, but the objections haven’t been addressed.

django-stubs is somewhat popular, most features are covered. It's not a feat of improved typing though, it's the fact that we started using runtime information from Django _meta API to infer types. So, it's not really a "static analysis" anymore.
I don't whether it's important though, it creates some issue with initial configuration for users, but so far it doesn't seem to be a roadblock.

[mkurnikov]

I've added a draft DEP/RFC for you and others to consider https://github.com/django/deps/pull/65

[rpkilby]

Hi @mkurnikov. I'm not that familiar with mypy since I've never been a fan of the type annotation syntax. However, the concept of stub files is new to me and seems interesting. Is the idea that we'd be adding these stub files to DRF, or is the expectation that we'd inline type annotations?

I guess my questions are:

• what changes are there to the code base (re: stubs vs inline annotations)
• do stubs live in the same directory or in a separate directory (latter seems preferable to me?)
• what is the CI workflow to ensure our annotations/stubs are up-to-date

And if it's not too much effort (and I mean very low effort, don't want to waste your time if this isn't going anywhere), could you throw together a preliminary PR? I'm thinking:

• a tox testenv to run whatever mypy/related checks are required
• a job in Travis to run the tox env
• annotations for one or two simple files

[mkurnikov]

@rpkilby

what changes are there to the code base (re: stubs vs inline annotations)

do stubs live in the same directory or in a separate directory (latter seems preferable to me?)

AFAIK, if stubs are in the same repo, they should be in the same directory, near the original file. But if they're going to remain in the stubs, they might as well just remain in the different package, like currently in djangorestframework-stubs.

what is the CI workflow to ensure our annotations/stubs are up-to-date

In the current state of mypy, there's no way to use stubs to typecheck code itself. My proposal to make it inline originated from this exact problem, djangorestframework-stubs become quickly stale. I test against the DRF tests, but i guess they don't cover all the calls.

There's a tools like merge-pyi,
https://github.com/google/pytype/tree/master/pytype/tools/merge_pyi
to merge stubs into code for CI, though. We can add merge + typecheck code into the CI to make them always up to date.

And if it's not too much effort (and I mean very low effort, don't want to waste your time if this isn't going anywhere), could you throw together a preliminary PR? I'm thinking:

a tox testenv to run whatever mypy/related checks are required
a job in Travis to run the tox env
annotations for one or two simple files

Most effort would be in tox, I never worked with it, but I'll try. Annotations are no-brainer, there's already done for most of the files, I'll just copy-paste them from djangorestframework-stubs.
PR will arrive sometime around the weekend though.

[rpkilby]

But if they're going to remain in the stubs, they might as well just remain in the different package, like currently in djangorestframework-stubs.

Well, it might be easier to keep them up-to-date if they were located in the DRF repo, but again, I'm not sure what the maintenance process is like so ¯\_(ツ)_/¯

Most effort would be in tox, I never worked with it, but I'll try.

tox is a great tool and I highly recommend learning it irrespective of this PR. Regardless, you're most likely going to want to do something that mimics the lint test env.
https://github.com/encode/django-rest-framework/blob/0fd72f17ee18506c02b4c0f0e4af368e3bedac3e/tox.ini#L46-L51

[tomchristie]

So, firstly - I took a look over the stub package, and I'm actually really impressed. Big chunk of work there, and looks nicer than expected.

Saying that I probably wouldn't want us to include stubs. If we're going to include typing information then it ought to be in the codebase itself.

I'm not totally convinced that DRF is a great target for adding typing information - I find type checking really useful in projects where it's enabled from the start, and is part of the CI process, ensuring that it's consistent when used.

It's less clear that it'd be valuable when backporting it to an existing large project which already has decent coverage, and very mature years-long exposure.

[mkurnikov]

@tomchristie
I can agree with the fact that adding type informations for Internal use as to catch bugs in the codebase and simplify learning curve to new contributors, wouldn't make much difference - DRF is very mature, typechecking won't achieve much.
But for External use, where users trying to adopt DRF for their needs, it could be of immense help sometimes. It's very nice to not making mistakes trying to pass arguments to

serializers.CharField()

for example.
With this https://github.com/typeddjango/djangorestframework-stubs/blob/master/rest_framework-stubs/fields.pyi#L107 kind of type/arguments information available, it spares user from looking up the docs from time to time.

Also, if codebase is typechecked (= mypy aware of the types), there's possibility to add more capabilities with a plugin for mypy.
See this, for example
https://github.com/typeddjango/djangorestframework-stubs/blob/e73110c5e35f9aae22f19fed4f71dcb440bd3619/test-data/typecheck/serializer_validation.test#L8
Plugins allows to infer type of serializer.run_validation() to be TypedDict of specific arguments and types.
Means to achieve those are kind of ugly though, I had to add private attributes for fields to denote which type use for __set__ and for __get__
https://github.com/typeddjango/djangorestframework-stubs/blob/e73110c5e35f9aae22f19fed4f71dcb440bd3619/rest_framework-stubs/fields.pyi#L53
But with core support, I'm sure there's a way to do it in a cleaner way.

UPD. Typechecking of serializer.run_validation() is actually the reason I've added DRF stubs in the first place.

[mkurnikov]

Here's the PR: https://github.com/encode/django-rest-framework/pull/6988