Looking much better, but there is still room for improvement. Passing a QVariantMap in the constructor is IMHO a violation of the law of demeter. The constructor should not make assumptions/rely on the content of args. It should ask directly for what it wants, that is a String body and a String address. I assume you want to be extensible this way, but adding a third property, say an image that is being sent would still need changes in the class. That would be a violation of the open-closed principle, that states that a class should be closed for modification but open for extension. Taking the example of an image message: Instead of cramming all the logic into Message one should create a class ImageMessage that extends Message and takes the image as a third constructor argument.

In D11854#238215, @nicolasfella wrote:

Looking much better, but there is still room for improvement. Passing a QVariantMap in the constructor is IMHO a violation of the law of demeter. The constructor should not make assumptions/rely on the content of args. It should ask directly for what it wants, that is a String body and a String address. I assume you want to be extensible this way, but adding a third property, say an image that is being sent would still need changes in the class. That would be a violation of the open-closed principle, that states that a class should be closed for modification but open for extension. Taking the example of an image message: Instead of cramming all the logic into Message one should create a class ImageMessage that extends Message and takes the image as a third constructor argument.

I don't disagree, but here is my thinking: Making a lot of individual arguments (this Message classs will need a few more, "type" at a minimum, and I like the concept of an ImageMessage) and passing them to the constructor offloads the logic of how a Message needs to be constructed from Message to the user. In other words, every place I want to construct a Message will have duplicate code. You're right that there is some worry about fragility by passing an unconstrained map of arguments.

Not sure if I'm overly nitpicky here 😆

Will a message be always constructed from a QVariantMap? If not it would make more sense to ask for the actual parameters. To avoid code duplication one could make a helper method like

Message Message::fromQVariant(QVariant args) {}

In D11854#248447, @nicolasfella wrote:

Looks like your diff got lost

Hopefully one day I will make arc do what I want on the first try :)

In D11854#248451, @nicolasfella wrote:

Not sure if I'm overly nitpicky here 😆

Will a message be always constructed from a QVariantMap? If not it would make more sense to ask for the actual parameters. To avoid code duplication one could make a helper method like

Message Message::fromQVariant(QVariant args) {}

I think that is a fair suggestion, but what does such a method offer vs. a constructor which takes a QVariantMap?

At the moment, the only way I see a Message being created is from a QVariantMap, since the only way to get a message is from a network packet, and network packets contain QVariantMaps

Test plan should include some steps to reproduce the desired behavior and see what is already implemented and what not

Would you consider (when you're ready) updating the README to be a little more complete, since you're probably quite familiar with the expected packet formats?

Hey careful there. You merged master instead of rebasing somewhere in between. Can someone clean the branch before merging?

In D11854#275000, @apol wrote:

Hey careful there. You merged master instead of rebasing somewhere in between. Can someone clean the branch before merging?

Are you sure? I rebased the branch onto master before using arc to push it here (but the branch is still in crazy land). How would I tell if it's messed up?

In D11854#275683, @sredman wrote:

In D11854#275000, @apol wrote:

Hey careful there. You merged master instead of rebasing somewhere in between. Can someone clean the branch before merging?

Are you sure? I rebased the branch onto master before using arc to push it here (but the branch is still in crazy land). How would I tell if it's messed up?

Okay, having just had my first experience landing a commit from the Contacts plugin, I now see what you're saying. Are you happy with what you're seeing here on Phabricator? I can land exactly this with no trouble.