review on_message jn MessageHandler

[antgonza]

This line is currently parsing text vs json, need to review it this is OK

[josenavas]

The line you're pointing to is Line num 2, which is a comment line. However, if yo meant line 29, it is using loads which is parsing JSON...

[antgonza]

I see and yes talking about that line. So basically msginfo['msg'] has
more than 1 field separated by :, right? Will it be possible to
better have more fields within the json?

[adamrp]

From what Josh told me yesterday, i think there is only one value in there,
"user: username", although i havent seen it with my own two eyes.
On Jun 21, 2014 5:24 AM, "Antonio Gonzalez" notifications@github.com
wrote:

I see and yes talking about that line. So basically msginfo['msg'] has
more than 1 field separated by :, right? Will it be possible to
better have more fields within the json?

—
Reply to this email directly or view it on GitHub
#153 (comment).

[squirrelo]

The issue actually references line 34 now.

This is used as a handshake between the user and the server, so on a websocket connection opening the user sends back the string "user:[USERNAME]" to open the connection. From there, only the server sends information back to the user as JSON. That's why this is set up in this manner.

[antgonza]

Why did you close this? Not sure the explanation actually solves the
problem? The problem is that we shouldn't be parsing this, right?

[squirrelo]

It's not really parsing anything as it's a handshake string. I closed it because it's a handshake that works effectively, unless someone can think of a better way to do it.

[antgonza]

The better way to do it is not to pass "user:" in the message.

[squirrelo]

I added that as a safety check, in case somehow a different message is passed back from the client.

[antgonza]

Well, if the client can send messages back it can add the 'user:', so not
sure how that improves anything? Anyway, if the idea is that messages are
going to contain other info why not create a function that actually does
that? Could you open the issue as it is not resolved?

[squirrelo]

It's in case other information needs to be sent back from the user, not just the handshake. This makes it so that specifically when the username is sent back it starts the process of registering the redis listener and other things required for the actual messaging to happen. Currently this doesn't send anything else back to the server, but in the future it may.

[antgonza]

or it may not, right? Again, this implementations is currently ugly so
either remove or improve it.

[squirrelo]

I don't understand what you mean.

[antgonza]

You are adding some extra text to a message as perhaps in the future it
will be used but you are not sure about this. Also, you are parsing it
inline vs. having a message controller/parser to take care of it (which is
ugly), right? So why not remove it or create the message controller/parser?

[wasade]

Agree with @antgonza here. The field is misleading at best as the field being check is "user:" but the interpretation is "here is the channel I'm listening on" or "acknowledged". If the intent is to structure this for future undefined messages, then that is fine, but it would be good to decompose it appropriately.

[squirrelo]

It's a bit muddled since the channel name is the user name (in this case email) but it's broken down in the pull request.