Reading notes, initial thoughts and refactoring suggestions #1312
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
Use Python 3 f-strings for readability. See https://www.python.org/dev/peops/pep-0498 and https://realpython.com/python-f-strings
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
gonzalo-bulnes
force-pushed
the
refactoring-notes
branch
from
efe431b to
c7d10b5
Compare
I don't know yet how to deal with the ArgumentParser expectations for expand_to_absolute.
I think it's good to have the magic strings grouped and visible.
gonzalo-bulnes
force-pushed
the
refactoring-notes
branch
from
c67ee36 to
a311b9b
Compare
If the goal of using the reverse DNS notation is to prevent naming conflicts, then I suppose we should use a domain nameunder the FPF control. Caution: I haven't found any other mention of this file name on GitHub, but I suspect this might be a breaking change? Search https://github.com/search?q=org%3Afreedomofpress+org.freedomofthepress
gonzalo-bulnes
force-pushed
the
refactoring-notes
branch
from
a311b9b to
e8a9453
Compare
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
gonzalo-bulnes
force-pushed
the
refactoring-notes
branch
from
d601658 to
80e13d0
Compare
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 tempaton 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 passsed 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
This was referenced Oct 19, 2021
|
We've merged everything from this PR except this commit. It is unclear what the best value would be, considering that a |
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
Thinking aloud as I read through the code to become familiar with it. These commits are meant to be conversation starters, and the idea is that we can extract any subset of them to compose any number of actual PRs. Note that I'm making them as I read, so they come in no particular order and each commit is meant to be self-contained.
Please share your thoughts in the diffs! (That's the entire point 🙂)
Checklist
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