Add typing hints #83
Add typing hints #83
Conversation
This is an attempt at add type hints to the codebase. I'm mostly doing this since I want to better understand some of the helper functions so as to expose a few and make them reusable. There's still a issues in `_format_envvars` and `_generate_nodes`. Both of them take an argument and pass it to a conflicting function.
|
Any feedback for this? |
|
I won't get to this before the weekend at the earliest, but thanks for doing this. Will definitely review |
There was a problem hiding this comment.
This is a good start. Some small comments inline but crucially, this is untested. Can you add the relevant changes to tox.ini and setup.cfg to enable this? Something like this tox.ini should do:
[testenv:mypy]
deps =
mypy
commands =
mypy sphinx_click
Likewise, you probably want something like this in setup.cfg:
[mypy]
python_version = 3.8
disallow_incomplete_defs = True
show_column_numbers = True
show_error_context = True
ignore_missing_imports = True
follow_imports = skip
check_untyped_defs = True
warn_unused_ignores = True
strict_optional = False
| from typing import List | ||
| from typing import Optional | ||
| from typing import Tuple | ||
| from typing import Union |
There was a problem hiding this comment.
Rather than importing the individual types, can you import typing and use that. i.e.
import typing as ty
from click import core
# ...
def _get_help_record(opt: core.Option) -> ty.Tuple[str, str]:
# ...There was a problem hiding this comment.
What's the purpose of aliasing Tuple to ty.Tuple?
I don't mind that it's longer, but I fear that using a custom name for these common types will hurt readability with those unfamiliar with this codebase.
There was a problem hiding this comment.
It's not aliasing, at least not for the classes - I tend to prefer importing modules rather than the classes themselves because it's less noisy. You could drop the aliasing of the module (so typing.Tuple but that seems unnecessarily verbose
| @@ -256,7 +269,7 @@ def _get_lazyload_commands(multicommand): | |||
| return commands | |||
There was a problem hiding this comment.
You missed some inner methods
There was a problem hiding this comment.
It's not clear to me what _get_lazyload_commands handles, since it doesn't seem to run for the projects I'm using sphinx.
Using MultiCommand does not match here.
|
Thanks! |
This is an attempt at add type hints to the codebase.
I'm mostly doing this since I want to better understand some of the
helper functions so as to expose a few and make them reusable.
There's still a issues in
_format_envvarsand_generate_nodes. Bothof them take an argument and pass it to a conflicting function.
There's a second commit to this PR which has a change of which I'm not fully sure. That's not a typing change, but an actual logic change. It does work for me, however, I can't really say it's correct. It does remove one typing error.