Add typing to git_util.py, metadata.py and config.py #391
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #391 +/- ##
==========================================
+ Coverage 59.83% 60.09% +0.26%
==========================================
Files 23 23
Lines 1449 1451 +2
==========================================
+ Hits 867 872 +5
+ Misses 582 579 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
danielhollas
force-pushed
the
types/git-utils
branch
from
9a32ff8
to
e139446
Compare
danielhollas
changed the title
danielhollas
force-pushed
the
types/git-utils
branch
from
608437e
to
34921b0
Compare
danielhollas
changed the title
There was a problem hiding this comment.
@danielhollas thanks! Just minor requests. I don't think there is too many #type: ignore, as you said they are mostly from other libraries.
| @@ -120,22 +134,22 @@ def git_clone(url, commit, path): | |||
|
|
|||
|
|
|||
| @dataclass | |||
| class GitPath(os.PathLike): | |||
| class GitPath(os.PathLike): # type: ignore | |||
There was a problem hiding this comment.
We may want to avoid to use ignore for subclassing by setting https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-disallow-subclassing-any
There was a problem hiding this comment.
Interesting. Still not sure I fully understand the issue. Let's see how often we need this before making this a global option.
| @@ -1,15 +1,17 @@ | |||
| """App metadata specification""" | |||
| from __future__ import annotations | |||
|
|
|||
| from configparser import ConfigParser | |||
| from configparser import ConfigParser, SectionProxy | |||
There was a problem hiding this comment.
| from configparser import ConfigParser, SectionProxy | |
| from typing import TYPE_CHECKING | |
| if TYPE_CHECKING: | |
| from configparser import ConfigParser, SectionProxy | |
| from pathlib import Path |
A minor nitpick, you are the expert on module loading performance, will this improve the loading efficiency?
There was a problem hiding this comment.
Not really, both of these are stdlib modules, which are generally very fast to import. More importantly, pathlib is getting imported all over the place anyway, and you would anyway need to import ConfigParser here.
Step towards #363. I am definitely not a typing expert, taking this work as an opportunity to learn. Would appreciate help to get rid of the
type: ignores (although some of them seems to be caused bydulwichpackage not providing types).