api: Add dataclass for update_message events. #18411
Conversation
MSurfer20
force-pushed
the
fix_message_embeds
branch
from
69f4f26 to
f5c479e
Compare
| @@ -5485,6 +5485,38 @@ class DeleteMessagesEvent(TypedDict, total=False): | |||
| stream_id: int | |||
|
|
|||
|
|
|||
| class UpdateMessagesEvent(TypedDict, total=False): | |||
There was a problem hiding this comment.
That is something I used as a temporary solution and need to fix(hence the current WIP status). Removing it causes error since message_ids, which is a compulary field is calculated much later in the code and the event TypedDict is created much before.
There was a problem hiding this comment.
@timabbott Something that I now remembered -- the total=False is necessary because we need to omit the Optional parameters completely rather than including them with None, since the API expects those keys to be absent rather than present with value None, and gives error in such cases.
A downside of doing this is that we won't be able to check for missing parameters in the event.(I followed the example of DeleteMessagesEvent to use total=False)
| new_stream_id: Optional[int] | ||
| orig_subject: Optional[str] | ||
| subject: Optional[str] | ||
| topic_links: Optional[List[Dict[str, str]]] |
There was a problem hiding this comment.
This is kinda giant; I wonder if there's a good way to define simpler classes; e.g. have the common elements in a base class, and then subclasses for "has topic edit"? Not sure; in any case, it's an improvement to just have a defined type even if it is giant.
There was a problem hiding this comment.
Also, ideally we'd use the class in zerver/tornado_event_queue.py if possible.
There was a problem hiding this comment.
This is kinda giant; I wonder if there's a good way to define simpler classes; e.g. have the common elements in a base class, and then subclasses for "has topic edit"? Not sure; in any case, it's an improvement to just have a defined type even if it is giant.
I initially tried to do it this way -- but there are total 4 branching ifs - if there is content, if it is a stream message, if the topic is changed or stream is moved, if the topic is changed - giving rise to 16 possible classes. That might make the class structure, as well as the code to use the appropriate class for the TypedDict a little cluttered in my opinion.
Also, in TypedDict, it is not possible to change type of an inherited parameter in the child class, so an approach where we have Optional elements made non-optional in child classes won't work out as well.
There was a problem hiding this comment.
@timabbott Would this large number of different classes be desirable? Since this is the only way I think we could remove the total=False if that is desired.
MSurfer20
force-pushed
the
fix_message_embeds
branch
3 times, most recently
from
6230034 to
5ad6d66
Compare
Different variants of update_message events made it difficult to keep api changes uniform across them. Created a dataclass for update_message variant events and refactored to replace Dict with TypedDict. Fixes zulip#16022.
MSurfer20
force-pushed
the
fix_message_embeds
branch
from
5ad6d66 to
d7cddad
Compare
MSurfer20
changed the title
|
Heads up @MSurfer20, 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 |
timabbott
force-pushed
the
main
branch
2 times, most recently
from
4ec3636 to
88b200c
Compare
timabbott
added
the
completion candidate
Different variants of update_message events made it difficult
to keep api changes uniform across them. Created a dataclass for
update_message variant events and refactored to replace Dict with
TypedDict. Fixes #16022.
Testing plan:
GIFs or screenshots: