-
Notifications
You must be signed in to change notification settings - Fork 216
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 poll start/end read receipts (PSB-183) #1633
Conversation
Codecov ReportBase: 36.22% // Head: 16.02% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #1633 +/- ##
============================================
- Coverage 36.22% 16.02% -20.20%
============================================
Files 582 583 +1
Lines 92408 92557 +149
Branches 40165 39036 -1129
============================================
- Hits 33471 14830 -18641
- Misses 57935 77242 +19307
+ Partials 1002 485 -517
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
kMXEventTypeStringPollStart, | ||
kMXEventTypeStringPollEnd, | ||
// unstable event types | ||
kMXEventTypeStringPollStartMSC3381, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be a good idea to add "Unstable" to the constant name instead of using a comment? E.g.
kMXEventTypeStringPollStartMSC3381Unstable
and kMXEventTypeStringPollEndMSC3381Unstable
. I am usually against comments in general because you have to maintain them, better to explain yourself with the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think is a good idea: having constants ending for MSCxywz
is already an hint about the event type being unstable.
The reason for the comment is to try to make people to group all the unstable event type together so that it would be easier to delete them when we'll need to do it.
I don't mind to delete the comment if you think isn't very helpful btw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I would remove the comment then, but it is up to you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we can live without it.
Removed here -> 2a6a1dc
This PR adds the sending of read receipts for the following event types: