refactor: Move from type comments to inline type annotations. #910
Conversation
ahmedabuamra
force-pushed
the
abuamra-inline-type-annotations
branch
2 times, most recently
from
9730596 to
50641de
Compare
There was a problem hiding this comment.
@ahmedabuamra Welcome to the project and thanks for looking into this 👍
The com2ann tool is automated so I've not looked in fine detail, but noticed a few cases we may want to look into in terms of formatting, checking the state of lister/run-mypy compared to zulip/zulip, and OrderedDict (as discussed on chat.zulip.org).
This does point to various cases where we could improve the typing - while equivalent I'd prefer Optional[X] to Union[None, X], for example - but we can address those in another PR.
tools/lister.py
Outdated
| result_dict: Dict[str, List[str]] = defaultdict(list) | ||
| result_list: List[str] = [] |
There was a problem hiding this comment.
I'd suggest we don't update tools at this point, at least not this file, since this file was ported from zulip/zulip and it would be useful to check separately if other updates are in order - and that repo has likely updated to inline types which we could pull across for comparison in any case. The same applies to run-mypy, which uses lister.
There was a problem hiding this comment.
I understand. tools/lister.py is the only changed file in tools.
zulipterminal/config/keys.py
Outdated
| @@ -11,7 +11,7 @@ class KeyBinding(TypedDict, total=False): | |||
| key_category: str | |||
|
|
|||
|
|
|||
| KEY_BINDINGS = OrderedDict([ | |||
| KEY_BINDINGS: Dict[str, KeyBinding] = OrderedDict([ | |||
There was a problem hiding this comment.
Let's stick with OrderedDict cases being in type comments for now. We can move over when we shift to python3.7+, if we still have OrderedDict around then.
zulipterminal/model.py
Outdated
| custom_emojis: NamedEmojiData = { | ||
| emoji['name']: { | ||
| 'code': emoji_code, | ||
| 'type': 'realm_emoji' | ||
| } | ||
| for emoji_code, emoji in response['emoji'].items() | ||
| if not emoji['deactivated']} |
There was a problem hiding this comment.
Did com2ann generate this new layout? We're moving towards a style where large bracketed code have the closing one on the starting line, though the original version isn't ideal in this respect either.
There was a problem hiding this comment.
Meaning to be like that, right?
custom_emojis: NamedEmojiData = {
emoji['name']: {
'code': emoji_code,
'type': 'realm_emoji'
}
for emoji_code, emoji in response['emoji'].items()
if not emoji['deactivated']
}
There was a problem hiding this comment.
Right 👍 Also trailing commas, but that's simpler to quickly fix :)
There was a problem hiding this comment.
@neiljp Done :)
zulipterminal/ui.py
Outdated
| footer_text: List[Any] = [' ' + s + ' ' | ||
| for s in suggestions] |
neiljp
added
area: refactoring
PR awaiting update
|
Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out! |
ahmedabuamra
force-pushed
the
abuamra-inline-type-annotations
branch
2 times, most recently
from
3289791 to
78148ee
Compare
|
@zulipbot remove "PR needs update" |
zulipbot
removed
the
PR awaiting update
|
To deal with types like KEY_BINDINGS: "OrderedDict[str, KeyBinding]" = OrderedDict([
…
]) |
ahmedabuamra
force-pushed
the
abuamra-inline-type-annotations
branch
from
78148ee to
819cb39
Compare
Ran `com2ann` in the project folder to auto re-write the files. Note: * tools/lister.py file has not been modified as imported from zulip/zulip Fixes: zulip#901
ahmedabuamra
force-pushed
the
abuamra-inline-type-annotations
branch
from
819cb39 to
efd4d0b
Compare
|
@ahmedabuamra Thanks for getting this done - I'm merging this now 🎉 |
Ran
com2annin the project folder to auto re-write the files.Fixes: #901