Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add reliability tests for direct-to-device messaging #295

Merged
merged 13 commits into from
Sep 2, 2016

Conversation

NegativeMjark
Copy link
Contributor

@NegativeMjark NegativeMjark commented Aug 25, 2016

Adds tests for matrix-org/synapse#1046


push our @EXPORT, qw( matrix_send_to_device_message );

sub matrix_send_to_device_message
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bold choice of name there. Maybe send_message_to_device; or perhaps is the "message" part implied? Could it be just send_to_device ?

@leonerd
Copy link
Contributor

leonerd commented Aug 25, 2016

Having got to the end I now see that to device message is in fact a compound noun, not part of the verb structure. I wonder if this code would be a bit more readable were these renamed to something else, perhaps a "device-directed message" or similar? If that name sounds a bit long, I don't think it would be unreasonable to document/specify these at the protocol level and introduce the initialism DDM to refer to it; then we can name functions

sub matrix_send_DDM
sub matrix_recv_DDM
sub matrix_recv_and_ack_DDM
etc...

@NegativeMjark
Copy link
Contributor Author

@leonerd I've called it a device_message for now since DDM is probably a bit meaningless for someone trying to read the code.

sub matrix_send_device_message
sub matrix_recv_device_message
sub matrix_recv_and_ack_device_message

@leonerd
Copy link
Contributor

leonerd commented Aug 26, 2016

LGTM

@NegativeMjark NegativeMjark merged commit 4171779 into develop Sep 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants