Add initial type hints and run mypy in CI

[intgr]

Closes #337.

[intgr]

This code is a bit iffy. err.detail isn't guaranteed to be a list of ErrorDetail. It can be an arbitrarily nested combination of list, dict and ErrorDetail.

[intgr]

rest_framework.compat.patterns doesn't actually exist, this crashes with ImportError.

[alanjds]

Overall, I would try to keep typehints that enhance the legibility, not that enhance automated type checking. However, some compromise is possible in this front.

Instead of using stuff from typing I would from __future__ import annotations and go with str | None and list in opposition of Union[str, None] and List[Any], for example.

Also, some parts for me confuses the reader more than helps them, like generics and so.

May be not the time to introduce Mypy as a linter. I am more leaned in typehints for humans.
I can contribute to this PR if you like, but am tight on time this last weeks.
Over and all, I really appreciate your the contribution.

[intgr]

Instead of using stuff from typing I would from future import annotations and go with str | None and list in opposition of Union[str, None] and List[Any], for example.

Indeed, I hate the Union[] things. I didn't realize that is supported on relevant Python versions. Will change.

[intgr]

Also, some parts for me confuses the reader more than helps them, like generics and so.

I think generics are useful for human understanding too.

For example this tells the story "each NestedHyperlinkedRelatedField instance is bound to a certain Model type (T_Model). The get_object() and to_internal_value() methods always return values of this declared model type (not some arbitrary model) and that also applies to inherited methods and attributes, e.g. RelatedField.get_queryset()"

May be not the time to introduce Mypy as a linter. I am more leaned in typehints for humans.

I can remove mypy linting from this repo. It was useful for double-checking that my added hints are correct. Though seems like a loss from my PoV.

But some 3rd party projects that use drf-nested-routers will want to use type checkers on their project. That was my motivation for adding type hints here in the first place, I use mypy in my own project. I would urge not to remove anything that would degrade typechecking accuracy for downstream users.

[alanjds]

For me, this PR is way more readable now. Am still concerned about some verbosity added but there is no much we can do about this iiuc.

Specifically about the casts, I would prefer to not include them, and mark only the result types instead.

Specifically about mypy, I am in favor of letting it running in a non-mandatory way for PR merging. Or at least in a way very easy to turn to non-mandatory in the future.

[intgr]

Specifically about the casts, I would prefer to not include them, and mark only the result types instead.

I replied in 2 of the threads, that doesn't solve the issue.

There are a few options: using cast() or a # type: ignore comment or disabling mypy entirely. (I also tried duplicating the signatures of DRF's methods, but that created more issues than it solved)

Specifically about mypy, I am in favor of letting it running in a non-mandatory way for PR merging. Or at least in a way very easy to turn to non-mandatory in the future.

How to implement it as "non-mandatory"? Just a separate GitHub job, but still reporting the success/error status? Or ignoring its status entirely and always reporting success?

[alanjds]

There are a few options: using cast() or a # type: ignore comment or disabling mypy entirely. (I also tried duplicating the signatures of DRF's methods, but that created more issues than it solved)

I see :/ . Well, I prefer to go with # type: ignore for now, as it does not confuse/clutter the code.

Specifically about mypy,...

How to implement it as "non-mandatory"? Just a separate GitHub job, but still reporting the success/error status? Or ignoring its status entirely and always reporting success?

A separate GitHub job is better, as is just a GitHub config to let it mandatory or not. This way, the success/error status can be reported correctly always.

[intgr]

1. Should these dependencies be included in requirements-tox.txt, as is now?
2. Should I create separate requirements-mypy.txt?
3. Should I embed them into tox.ini?

[alanjds]

I am fine with any of the options, yet less files seems preferable.

[intgr]

I have split mypy into a separate job and removed some hints according to feedback.

If what you mean by "non-mandatory" is jobs without these "Required" labels, then this is actually part of the repository admin confguration in GitHub web UI, not part of the workflow files. Jobs are "non-required" by default unless explicitly added there. So that part should be solved.

[alanjds]

That is great. Thanks a lot for the effort @intgr and the patience on the code review roundtrips.

[intgr]

Thanks! Before this is usable for downstreams, we still need to add py.typed file, so don't release this yet (in case you were planning to). 😄

[alanjds]

FYI: Is just released on PyPI a minor version with this PR included: https://pypi.org/project/drf-nested-routers/0.94.0/

[alanjds]

so don't release this yet (in case you were planning to). 😄

Oops. Looks like I will need to release a patch very soon.