actions: Refactor check_message to return a dataclass instead of Dict. #16786
Conversation
sahil839
force-pushed
the
message-dict
branch
from
04aaf7b
to
584e27e
Compare
sahil839
changed the title
zerver/lib/actions.py
Outdated
| already_sent_ids: List[int] = [] | ||
| new_messages: List[MutableMapping[str, Any]] = [] | ||
| for message in messages: | ||
| if isinstance(message['message'], int): | ||
| already_sent_ids.append(message['message']) | ||
| else: | ||
| new_messages.append(message) | ||
| messages = new_messages |
There was a problem hiding this comment.
There was a problem hiding this comment.
@andersk I have updated the PR. I have changed the caller of check_message to return the message-id directly when the exception is raised instead of returning it via do_send_messages as we were doing it previously to avoid using MessageInfo dataclass for this case.
The checks are failing because of coverage issue and I will fix this. Does this approach seem fine to you or we should do something else?
There was a problem hiding this comment.
Also just wanted to confirm, we would not these checks for check_send_stream_message and check_send_private_message as these are just used by webhooks and tests, and from what I have read in the Integrations section of https://zulip.readthedocs.io/en/latest/subsystems/client.html, I can understand that client cannot be zephyr_mirror for messages sent through webhooks. So, the try..except block will be needed in check_send_message only and can be removed from other places.
sahil839
force-pushed
the
message-dict
branch
2 times, most recently
from
6a5cf87
to
ce14df6
Compare
|
LGTM. |
sahil839
force-pushed
the
message-dict
branch
from
ce14df6
to
500efec
Compare
zerver/lib/actions.py
Outdated
| @@ -1691,7 +1682,7 @@ def do_send_messages(messages_maybe_none: Sequence[Optional[MutableMapping[str, | |||
| # returned. In practice, this shouldn't matter, as we only | |||
| # mirror single zephyr messages at a time and don't otherwise | |||
| # intermingle sending zephyr messages with other messages. | |||
| return already_sent_ids + [message['message'].id for message in messages] | |||
There was a problem hiding this comment.
The above comment is no longer accurate and can be deleted with this commit; I'll take care of that.
There was a problem hiding this comment.
Yes, missed that.
timabbott
force-pushed
the
message-dict
branch
from
500efec
to
c24ee07
Compare
|
Overall, this looks great! I merged a few prep commits as the series ending with 37c8505. My only concern at present is the naming for this new structure. I think we should do |
sahil839
force-pushed
the
message-dict
branch
3 times, most recently
from
6ca8fa5
to
b0e1220
Compare
… Dict. We change the return type of check_message to be dataclass instead of Dict[str, Any]. This refactoring helps us to understand the context of the data structure returned by check_message clearly which was not possible when using Dict. SendMessageRequest class is added in zerver/lib/message.py inspite of it not being used in that file itself just to maintain consistency as other TypedDicts and dataclasses are defined in that file and to avoid circular dependency as SendMessageRequest is being used in lib/widget.py as well. We also rename local variable to 'send_request' for accessing SendMessageRequest objects.
sahil839
force-pushed
the
message-dict
branch
from
b0e1220
to
caa0475
Compare
|
@timabbott Updated the PR. Renamed the class to |
This PR refactors the
check_messagefunction inzerver/lib/actions.pyto return a dataclass instead of Dict.Also added few prep commits.
Testing plan: Modified the tests.