Add basic mypy setup to tox.ini #6988
Conversation
|
Thanks for throwing this together - I really appreciate it. |
| -rrequirements/requirements-documentation.txt | ||
|
|
||
| [testenv:mypy] | ||
| basepython = python3.7 |
|
@mkurnikov flake8 has been update in by @rpkilby with my last PR on master, you can try to rebase |
carltongibson
left a comment
carltongibson
left a comment
There was a problem hiding this comment.
Hi @mkurnikov. Thanks for this.
I left a few comments inline, which are questions really.
Then:
- Why the
# type:comments, rather than annotations? - And, do we need these to begin? I didn't run mypy yet to see, but these are the minimum?
Thanks again!
| import yaml | ||
| except ImportError: | ||
| yaml = None | ||
| yaml = None # type: ignore |
There was a problem hiding this comment.
Why do we need this?
| 'empty': _('This selection may not be empty.') | ||
| } | ||
| default_empty_html = [] | ||
| default_empty_html = [] # type: List[Any] |
There was a problem hiding this comment.
OK, so can'y mypy just infer this? Surely all lists are List[Any], like the = [] already implies that no?
There was a problem hiding this comment.
It looks like mypy can't infer the type of empty lists, so that may be why it was specified here.
There was a problem hiding this comment.
To clarify, it looks like MyPy follows the "Explicit is better than implicit" portion of the Zen of Python and doesn't assume List[Any] when it's not specified and the list is empty.
There was a problem hiding this comment.
Sure.
(Grumble, grumble... elsewhere lack of an annotation is interpreted as Any... 🙂)
| child = _UnvalidatedField() | ||
| initial = {} | ||
| child = _UnvalidatedField() # type: Field | ||
| initial = {} # type: Dict[str, Any] |
There was a problem hiding this comment.
Again here, I'm a bit like, can't we just stick with = {}. I appreciate the str restricts the keys some but... ?
| media_type = None | ||
| format = None | ||
| charset = 'utf-8' | ||
| media_type = None # type: Optional[str] |
There was a problem hiding this comment.
Is there not a ? shortcut for Optional (like they have in Swift)? (Just wondering...)
There was a problem hiding this comment.
As far as I am aware, there isn't a shortcut for Optional.
|
I appreciate the effort, but I probably think we’re adding more noise than value here. I think type checking is great if you’re able to apply it consistently and throughout to a project from the start, but I tend to see it giving less value when backporting it onto established projects. |
|
For me, it depends how this conversation goes on Django. If we go for it there then... But the queries I have here are pertinent to that discussion. It's good to see what it'd look like. (IRL I often end up chucking in an odd |
|
I'm generally +0 on type annotations here. I agree that there's way more value in starting off projects with them, but I also believe that strategically adding them to existing projects can be useful. DRF still supports Python 3.5 so we can't have the clean variable type annotations that were adopted in Python 3.6. I'm willing to accept the compromise with the comment-based type annotations, even though I think they aren't as nice to look at. |
DRF isn't just a project, it's also a library and its type annotations can be used to type check projects that use DRF. (Type annotations alone are not enough for that, but most of the work for PEP 561 compat.)
If someone else is looking for the current discussion, there's a Django Enhancement Proposals: django/deps#65 |
7 participants
Here's the PR with just enough type annotations to allow mypy to run cleanly.
flake8warnings could be fixed by updating to the latest version.@rpkilby
toxreally is amazing=)