add support for extended addressing (xep-0033) #185
Conversation
| @@ -27,3 +27,11 @@ | |||
| event_client :: any(), | |||
| props :: list() | |||
| }). | |||
|
|
|||
| -record(extaddress, { | |||
There was a problem hiding this comment.
Maybe I'm limited or sth but I think that using a map here would simplify developer's life when it comes to debugging (which is quite common part of our day to day life :) ). The map could have it's own type with required and optional type.
There was a problem hiding this comment.
I have nothing against maps, they are so useful when you need flexibility. But when I know for sure what the data structure is supposed to contain I prefer records, they make code so much more readable.
There was a problem hiding this comment.
they make code so much more readable
I'd say the same about maps :) plus they make debugging so much nicer :)
There was a problem hiding this comment.
Do they give you code completion?
There was a problem hiding this comment.
Actually I don't use code completion so I don't care :)
There was a problem hiding this comment.
This case is not a big deal, so if it is your strict requirement then fine, I can change it to a map. But, really, we should give it a serious though. From what I know, our coding guidelines do not say "never use records", but the practice seems to be heading in that direction. I'd suggest we make some effort to formulate a common approach, stating which data structure should/can be applied in which cases, to avoid such tensions in the future.
There was a problem hiding this comment.
It's not a strict requirement, note that I didn't request for changes, just commented in the review :)
There was a problem hiding this comment.
Ok, got it. Can it be merged, then?
|
Did you try it with current MongooseIM tests? Please rebase this branch on top of current master (there was quite many changes in escalus recently) and open a PR in MongooseIM with escalus from this branch. |
bartekgorny
force-pushed
the
multicast
branch
from
ae76330
to
220f50e
Compare
|
Reviving this one, old and with some conflicts by now but I like the idea, would be cool to implement so in MongooseIM as well so having the testing tool will be useful beforehand. Any thoughts on this? @bartekgorny |
You may want to add some syntactic sugar, depends on your taste - for now the interface is a bit raw, you have to pass a tuple containing multicast host and a list of #extaddress{} records.