Use map for mod_mam:message_row() #3093
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #3093 +/- ##
==========================================
- Coverage 79.08% 79.03% -0.06%
==========================================
Files 386 387 +1
Lines 31829 31838 +9
==========================================
- Hits 25172 25163 -9
- Misses 6657 6675 +18
Continue to review full report at Codecov.
|
arcusfelis
force-pushed
the
mu-mam-message-row-is-map
branch
2 times, most recently
from
fe97255
to
2787246
Compare
arcusfelis
force-pushed
the
mu-mam-message-row-is-map
branch
from
2787246
to
e08c405
Compare
arcusfelis
changed the title
arcusfelis
force-pushed
the
mu-mam-message-row-is-map
branch
from
7ad1cec
to
ba664d7
Compare
NelsonVides
reviewed
Apr 26, 2021
NelsonVides
reviewed
Apr 26, 2021
NelsonVides
approved these changes
Apr 27, 2021
chrzaszcz
reviewed
Apr 27, 2021
There was a problem hiding this comment.
Looks good. I have a few comments and Elvis has two that I agree with:
mod_mam.erl 460 1 warning invalid_dynamic_call Remove the dynamic function call on line 460. Only modules that define callbacks should make dynamic calls. (lsp)
mod_mam.erl 617 1 warning line_length Line 617 is too long: message_row_to_xml(MamNs, #{id := MessID, jid := SrcJID, packet := Packet}, QueryID, SetClientNs) ->. (lsp)
src/mam/mam_lookup.erl
Outdated
| @@ -158,7 +158,8 @@ decode_rows(MessageRows, Env) -> | |||
| [decode_row(Row, Env) || Row <- MessageRows]. | |||
|
|
|||
| %% First element of the tuple is decoded message ID | |||
There was a problem hiding this comment.
There is no tuple now, please amend this comment.
src/mam/mod_mam.erl
Outdated
| @@ -119,7 +118,7 @@ | |||
|
|
|||
| -type borders() :: #mam_borders{}. | |||
|
|
|||
| -type message_row() :: {message_id(), jid:jid(), exml:element()}. | |||
| -type message_row() :: #{id => message_id(), jid => jid:jid(), packet => exml:element()}. | |||
There was a problem hiding this comment.
Are they optional? Could we make the keys mandatory in this type?
chrzaszcz
reviewed
Apr 27, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses "what if we use map, will tests still pass?"
Proposed changes include:
As a dev, I want to extract some extra fields from MAM (or during MAM lookup).
I am currently only allowed to return message-id, source-jid and stanza packet. But sometimes I need to return more fields, so I can format them later (to build a proper forwarded message). Maps allows to attach some meta info to each message.
Also, it's really hard to track what is what in 3 fields tuple