Complete type annotations for Qtile class #2465
Conversation
|
I'm very interested in hearing arguments against increasing our type annotation coverage if anyone is particularly against type checking in Python. I had made the assumption that as we have some, we'd probably want to increase this coverage. Indeed working on the Wayland backend I've found myself needing to figure out types by the code where there weren't annotations and I've made some type mistakes due to uncertain types. As a result I've grown to appreciate type hinting more, hence this PR. But anyway as I'm only recently a fan of Python typing I'm still eager to hear arguments against :) |
|
I'm far to be a type hints fan but that's the way it is so let's push it. :) |
m-col
force-pushed
the
typing
branch
3 times, most recently
from
e5dfc9f
to
0221a31
Compare
This lets us avoid using `hasattr` to check the existence of these methods before calling them from their respective cmd_s on `Qtile`. This pleases mypy, and also means that the base `Core` is truly The One place to look for possible methods that a core can have, which... makes sense.
`Window.togroup` is a required method, as picked up by mypy.
|
Rebased. |
| # one group per screen is needed | ||
| if len(self.groups) == len(self.screens): | ||
| raise ValueError("Can't delete all groups.") | ||
| if name in self.groups_map.keys(): | ||
| group = self.groups_map[name] | ||
| if group.screen and group.screen.previous_group: | ||
| if group.screen and hasattr(group.screen, "previous_group"): |
There was a problem hiding this comment.
This looks like a bit odd, is there a reason the old way of having previous_group be an optional would not work with the mypy tests?
There was a problem hiding this comment.
It does look a bit odd, but it's either this or peppering asserts throughout the code, which IMO makes things unclear as to why asserts are lying around the code, as well as messy and executing unnecessary code. Same with self.qtile: Qtile = None that we have on a number of classes, which results in us having lots of lines like here: https://github.com/qtile/qtile/blob/master/libqtile/backend/x11/core.py#L274
The hasattr lines are only required in the few places where these attributes can be accessed before getting populated, which in the case of self.qtile on things like windows pretty much never happens, but it can happen on Screen as you see here with self.previous_group.
To be honest I'm not sure if this is just a weakness in Python, or mypy, and it's probably just down to preference, so I'm happy to do whatever with these ones.
libqtile/core/manager.py
Outdated
| def reserve_space(self, reserved_space, screen): | ||
| def reserve_space( | ||
| self, | ||
| reserved_space: List[int], # [left, right, top, bottom] |
There was a problem hiding this comment.
It might be good to have this be Tuple[int, int, int, int], since it is required to have length 4. Similarly below.
There was a problem hiding this comment.
Yep sounds good. I've updated these tuples in a few places, but had to add an ignore comment due to python/mypy#7509. I've put an inline comment beside it so that in the future it can be checked and removed when mypy resolves this.
This adds type annotations to all attributes and methods of the `Qtile` class. Making these changes revealed some things that needed to be changed (i.e. things that would have failed type checking if these annotations existed first), and these changes are included here too: - `urgent` property on base `Window` class - `info` method on base `Window` class - `screen` attribute on base `Static` class This commit also takes advantage of class instance attribute type annotations that do not require assigning a value immediately. This lets us get around the case where we have an attribute that starts off as `None` in `__init__` only to become `not None` in `_configure` and remain as something else during normal runtime. Examples of this are `Screen.group`, or `Qtile.current_screen`. This means we not longer have to check to see if these are `not None` when they never will not be None, simply to appease mypy. Further changes could perhaps apply this approach to the various classes that have `self.qtile` and are peppered with `assert self.qtile is not None` lines. Further, to let mypy actually run on `Qtile.config` - which has dynamically set attributes based on possible configuration variables - the possible attributes are also type annotated as instance attributes. This replaces the existing `Config.settings_keys` and its use for `qtile check`.
Three commits here. The third is the main one; the first two were actually the result of adding type annotations to
Qtileand finding a couple things that needed changing because mypy was complaining so I separated those out before committing all the little annotations in the main commit. Together these commits add full type annotations toQtileand a few superficial changes to make mypy happy about them. The first two commits are self-explanatory but the third one has a bit more meat. See its commit message.