refactor: Add and use type annotations for urwid Text markups #1101
Conversation
This commit adds a general annotation for urwid.Text markup content. Since the urwid markup is used almost throughout the UI part of the codebase, it would be nice to annotate it as we move ahead with slightly complex UI features and widgets.
This commit uses the new markup type annotation to add type hints and annotations to important UI methods, such as `soup2markup`, `transform_content`, and annotates other urwid.Text markup variables.
This commit updates and simplifies the general PopUpViewContent type annotation, and annotates other urwid text markups as usual.
Ezio-Sarthak
added
area: infrastructure
|
Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out! |
There was a problem hiding this comment.
@Ezio-Sarthak This looks great. 👍
I had a couple of doubts related to the new types since they got really complex in a few places. This may not need updating depending on the legitimacy of my doubts below, but you'll need to rebase to get around with conflicts in any case. :)
| @@ -1079,11 +1080,13 @@ def footlinks_view( | |||
| def soup2markup( | |||
| cls, soup: Any, metadata: Dict[str, Any], **state: Any | |||
| ) -> Tuple[ | |||
| List[Any], "OrderedDict[str, Tuple[str, int, bool]]", List[Tuple[str, str]] | |||
| List[urwidTextMarkup], | |||
There was a problem hiding this comment.
Do we need to wrap urwidTextMarkup inside a List?
urwidTextMarkup already contains an outer layer of Sequence type which should cover List?
There was a problem hiding this comment.
I'm skeptical about this too. I think Sequence might only be applicable as a type if the contents in the sequence are not changing? (I think I discussed this on stream too?)
| @@ -944,7 +944,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]: | |||
| return super().keypress(size, key) | |||
|
|
|||
|
|
|||
| PopUpViewTableContent = Sequence[Tuple[str, Sequence[Union[str, Tuple[str, str]]]]] | |||
| PopUpViewTableContent = Sequence[Tuple[str, urwidTextMarkup]] | |||
There was a problem hiding this comment.
Similarly here, shouldn't just urwidTextMarkup suffice? Am I misunderstanding something?
|
Heads up @Ezio-Sarthak, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
What does this PR do?
Adds and uses type annotations for
urwid.Textmarkups.Tested?
Commit flow
urwid.Textmarkups.Notes & Questions
Interactions
Visual changes