add support for extended addressing (xep-0033) #185
Conversation
| 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 |
|
Not closing, because @NelsonVides is interested in it... |
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.