-
Notifications
You must be signed in to change notification settings - Fork 428
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
Room EventData is map now #3111
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3111 +/- ##
==========================================
- Coverage 79.22% 79.19% -0.03%
==========================================
Files 388 388
Lines 31858 31846 -12
==========================================
- Hits 25238 25220 -18
- Misses 6620 6626 +6
Continue to review full report at Codecov.
|
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.
Generally love this change, thanks for the quick action! Just a few minor comments 🙂
CHANGELOG.md
Outdated
- Use a map instead of a proplist in the `room_event_data` hook | ||
for the event data information. | ||
- `room_send_packet` hook removed. | ||
Use `filter_room_packet` instead. | ||
|
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 this is the place to put these things, they should go to the proper migration file
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 am just making less work for people, who need to write a changelog for the next release.
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.
Looks awesome to me 🎉
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.
Ah, forgot, the changelog, I didn't mean to remove it, I meant to put it on a new migration file, in the doc/migrations/ dir, as we always do
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.
👌🏽
This PR addresses "some tech debt".
Proposed changes include: