Type fixes #84
Type fixes #84
Conversation
| self.layers = [{}, {}, {}, {}, {}, {}] | ||
| self.dk_set = set() | ||
| self.dead_keys = {} # dictionary subset of DEAD_KEYS | ||
| self.layers: Dict[Layer, Dict[str, str]] = {layer: {} for layer in Layer} |
There was a problem hiding this comment.
This change is weird but seemed necessary for consistency.
There was a problem hiding this comment.
I wrote that too late yesterday. Thinking about it this morning under my shower, it is no longer weird. self.layers used to be a list accessed as a Dict[Layer, Dict[str, str]] because you can convert a Layer into an int and use as index of a list.
There was a problem hiding this comment.
This line is very self-explanatory, but what’s the point in using a Dict rather than a List here ? Doesn’t that defeat the idea of a Layer enum ?
And if we follow that route, I think that for layer in Layer could be more readable, maybe for index in Layer or something.
There was a problem hiding this comment.
Indeed this is roughly saying we want an Enum instead of IntEnum. My modification makes things easier for typechecking (mypy certainly complained about the original version) and I think it's more readable. I don't think the ordering implied by the use of IntEnum is relevant here. One can argue that shift layers come after their regular version but I don't see why ODK and ALTGR would be ordered.
There was a problem hiding this comment.
Makes sense. I guess I tried to apply an old-school mental model I got from C here.
| @@ -338,7 +338,7 @@ def klc_dk_index(layout: "KeyboardLayout") -> List[str]: | |||
| # | |||
|
|
|||
|
|
|||
| def osx_keymap(layout: "KeyboardLayout") -> List[str]: | |||
| def osx_keymap(layout: "KeyboardLayout") -> List[List[str]]: | |||
There was a problem hiding this comment.
This return type looks a bit suspicious, but this is what the function does.
| with file_path.open(mode="rb") as file: | ||
| return tomli.load(file) | ||
| with file_path.open(mode="rb") as dfile: | ||
| return tomli.load(dfile) |
There was a problem hiding this comment.
out of curiosity, why dfile rather than file ? Is file a known type or something ?
There was a problem hiding this comment.
Not at all, the issue is the collision with the previously defined file variable that is overwritten and had a different type, hence the type checker complains. I know everybody expect that file is no longer in scope after the end of the with block, but this simply not how python works. See this stackoverflow discussion for instance.
| self.layers = [{}, {}, {}, {}, {}, {}] | ||
| self.dk_set = set() | ||
| self.dead_keys = {} # dictionary subset of DEAD_KEYS | ||
| self.layers: Dict[Layer, Dict[str, str]] = {layer: {} for layer in Layer} |
There was a problem hiding this comment.
This line is very self-explanatory, but what’s the point in using a Dict rather than a List here ? Doesn’t that defeat the idea of a Layer enum ?
And if we follow that route, I think that for layer in Layer could be more readable, maybe for index in Layer or something.
| @@ -174,7 +195,7 @@ def __init__(self, filepath: Path) -> None: | |||
| self.meta["lastChange"] = datetime.date.today().isoformat() | |||
|
|
|||
| # keyboard layers: self.layers & self.dead_keys | |||
| rows = GEOMETRY[self.meta["geometry"]]["rows"] | |||
| rows = GEOMETRY[self.meta["geometry"]].rows | |||
There was a problem hiding this comment.
That’s quite exactly what I wanted to see from these data classes.
I’ll try to use that for meta later today.
| return Layer.BASE | ||
| elif self == Layer.ODK_SHIFT: | ||
| return Layer.SHIFT | ||
| return self |
There was a problem hiding this comment.
Nice. The name is totally debatable but I love this idea.
There was a problem hiding this comment.
Yes it was a bit late when I designed this joke. Feel free to revert of course. The thing that I think really matters is to have a clear function with clear meaning instead of a random -2.
There was a problem hiding this comment.
Hahaha I feel so stupid. Thanks for the good catch. :-)
| base: str | ||
| alt: str | ||
| alt_space: str | ||
| alt_self: str |
There was a problem hiding this comment.
Correct. No idea if this is doable with Python, but base and alt are actually char arrays rather than strings : they both contain exactly N characters and alt[i] is the altered version of base[i] for any i between zero and N-1.
There was a problem hiding this comment.
Weirdly enough, Python does not have a char type. I never learned why. So we could have List[str] here but not List[char].
| @@ -436,7 +436,7 @@ def append_actions(symbol, actions): | |||
| continue | |||
|
|
|||
| key = layout.layers[i][key_name] | |||
| if i and key == layout.layers[0][key_name]: | |||
| if i and key == layout.layers[Layer.BASE][key_name]: | |||
| @@ -359,13 +381,12 @@ def _fill_template( | |||
|
|
|||
| return template | |||
|
|
|||
| def _get_geometry(self, layers: Union[List[Layer], None] = None) -> str: | |||
| def _get_geometry(self, layers: Optional[List[Layer]] = None) -> List[str]: | |||
There was a problem hiding this comment.
First Optional I see in Python code. Loving it already.
| if shift_key != " ": | ||
| self.layers[layer_number + 1][key] = shift_key | ||
| self.layers[layer_number.next()][key] = shift_key |
There was a problem hiding this comment.
This looks like a very valid reason to kill my beautiful vertical alignment. Thanks for the expressiveness.
| alt_self: str | ||
|
|
||
|
|
||
| DEAD_KEYS = [DeadKeyDescr(**data) for data in load_data( "dead_keys.yaml")] |
There was a problem hiding this comment.
OMFG I hope I can remember this ** trick.
There was a problem hiding this comment.
That's one more reason why dataclass is nice. If your data easily fits into a json/yaml dict then they get very easy to (de)serialize.
There was a problem hiding this comment.
This becomes so expressive I want to cry. ^_^
Assorted type annotations fixes. In order to work reliably, type annotations need to get rid of all the "dictionary bags". So this PR also introduces more structured data types for
DEAD_KEYSandGEOMETRY. It also reduces the number of abuse of the fact thatLayerobjects are secretlyint.Note: I tried to produce reviewable individual commits, but this required some history rewriting because I was not rigorous enough when editing. It should be mostly ok except for a couple of phantom line deletions that should be ignored in early commits.