Add mypy static type checker to CI and necessary fixes to the source code #1717
Conversation
DavidMStraub
force-pushed
the
types_squash
branch
from
9951a0b
to
40c041b
Compare
| from ._reportdialog import ReportDialog | ||
|
|
||
| from ._papermenu import PaperFrame |
There was a problem hiding this comment.
I don't understand some of these new blank lines.
There was a problem hiding this comment.
Possibly added by an automatic formatter? There are other changes that solely alter the code's formatting.
|
Thanks for your review @QuLogic. This is now ready for merge. Then I can work on adding type hints. |
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
This reverts commit 0f5661f.
DavidMStraub
force-pushed
the
types_squash
branch
from
1c9001c
to
d685608
Compare
|
Thanks @QuLogic, I address the latest 2 comments and rebased on |
|
I'm planning a maintenance release this weekend. There are a few bugs fixed and two new translations added. After that I'll merge some of the PR s into master. |
|
This comment about |
|
Right, now it should be resolved. |
| @@ -42,7 +43,7 @@ class GT0(GrampsType): | |||
| # A migration utility might instantiate several of these with | |||
| # varying blacklist-specs | |||
| class GT1(GT0): | |||
| _BLACKLIST = BLIST | |||
| _BLACKLIST: list[int] | None = BLIST | |||
There was a problem hiding this comment.
Instead of typing on list (and dict, both in general and not just on this particular line), can we instead type on collections.Sequence, collections.MutableSequence, collections.Mapping, and collections.MutableMapping? In my experience it makes things slightly less dependent on internals (one historical example was dict being ordered as an internal implementation detail until... 3.7?). Concrete benefits are also that return values can be marked immutable, reducing the risk of other code attempting to modify data that should be read-only.
| @@ -646,7 +647,7 @@ def __init__(self, rlocale): | |||
| super().addLabels(loc) | |||
|
|
|||
| else: | |||
| AlphabeticIndex = localAlphabeticIndex | |||
| AlphabeticIndex = localAlphabeticIndex # type: ignore | |||
There was a problem hiding this comment.
mypy allows a # type: ignore[$reason-code] syntax as well where $reason-code is the code (readable machine name) of the specific violation you want to ignore. Doing that, in combination with enabling warn_unused_ignores in your favorite mypy configuration file means that you won't end up accidentally ignoring the wrong things as the codebase changes over time.
|
@bartfeenstra thanks for yet another review, but to reiterate, the sole purpose of this PR is to add type checks to the CI and add the minimal set of type hints that silence the type checker. It is NOT a PR that adds consistent, complete, or meaningful type hints to the Gramps code base. So I'm not willing to spend more work on this. I will wait for it to be merged and then start adding type hints in follow-up PRs. |
|
When the Type Hints start being added, please add a line to that Pull Request/Discussion to maintain a path for people who have a vested interest in the implementation Here's a related MantisBT issue: |
This adds mypy static type checks to CI. The PR contains the minimal set of changes to the code base needed to make mypy pass. Type hints are only added where needed by mypy, in most cases class/instance attributes initialized to empty lists or dicts. Some smaller bugs or inconsistencies discovered by mypy are also fixed.