alanjds / drf-nested-routers

Nested Routers for Django Rest Framework
https://pypi.org/project/drf-nested-routers/
Apache License 2.0
1.66k stars 157 forks source link

Add initial type hints and run mypy in CI #339

Closed intgr closed 4 months ago

intgr commented 4 months ago

Closes #337.

intgr commented 4 months ago

Marking as Draft until #340 is merged.

intgr commented 4 months ago

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 commented 4 months ago

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()"

image

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.

intgr commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

image

alanjds commented 4 months ago

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

intgr commented 4 months ago

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 commented 4 months ago

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

alanjds commented 4 months ago

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

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