Add type hints, enable Mypy checks for all production code #1313
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I came to really appreciate from Go that the type hints in function signatures are often sufficient to understand what each of the parameters and return values are. Besides allowing precise type-checking, that allows local variable names to become simpler (because their type conveys information about what they are already). Additionally, while I haven't implemented that idea systematically yet, I find interesting that custom types can be defined to make sense to anyone familiar with the domain, and used to facilitate cross-functional collaboration through shared vocabulary. See Domain Modeling Made Functional by Scott Wlaschin https://www.youtube.com/watch?v=2JB1_e5wZmU As far as I understand, typing.NewType is safe to introduce gradually because the variable will be cast automatically when str is required. That allow to benefit from type checking where LanguageCode is required, while keeping the change local. See https://www.python.org/dev/peps/pep-0484#newtype-helper-function
I don't know yet how to deal with the ArgumentParser expectations for expand_to_absolute.
Specifying when functions do not return allows Mypy to warn when unreachable code is introduced. Note: that feature is enabled by the option --warn-unreachable which is not currently used. I also noticed that enabling type ceking on this file uncovers errors in the GUI code, so I'll address those before modifying the Mypy options. See https://www.pyhton.org/dev/peps/pep-0484#the-noreturn-type
I added "ignore" comments when hinting at functions types
was necessary (don't know how to do that yet), or I wasn't
sure about what an approriate hint would be.
Examples: window, data, parent sometimes, datetimes...
In the face of ambiguity, refuse the temptation to guess. ;)
Interesting cases:
- logic.Controller.authenticated_user: without the type hint, some
code futher down was deemed unreachable by Mypy, likely because
it inferred a User type that always evaluates to True
- gui.main.Window.login_dialog: type checking FTW! That could have
been a runtime error.
- gui.widgets.SourceList.get_selected_source: that one could have been
a runtime error too.
- gui.widgets.SourceListWidgetItem: the class definition contains
a reference to itself in a type hint. Can be passed as a string.
See https://www.python.org/dev/peps/pep-0484#forward-references
See https://doc.qt.io/qt-5/qwidget.html
and https://adamj.eu/tech/2021/05/25/python-type-hints-specific-type-ignore
Co-Authored-By: Allie Crevier <[email protected]>
sssoleileraaa
approved these changes
Oct 19, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #25
See also: #1300 and #1312
It's also mostly about me getting familiar with Mypy. 🙂
Test Plan
2426 of them)# type: ignore ...comments to ensure they're reasonnableChecklist
If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:
If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:
If these changes modify the database schema, you should include a database migration. Please check as applicable:
mainand confirmed that the migration applies cleanlymainand would like the reviewer to do so