Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Message spec consistency #7

Closed
rgbkrk opened this issue Jul 16, 2015 · 9 comments
Closed

Message spec consistency #7

rgbkrk opened this issue Jul 16, 2015 · 9 comments

Comments

@rgbkrk
Copy link
Contributor

rgbkrk commented Jul 16, 2015

In looking at messages from jmp, I noticed that the parent header was camelCase while the spec is under_scores (http://jupyter-client.readthedocs.org/en/latest/messaging.html). This leads to amusing mixes like so:

message.parentHeader.msg_id

/cc @minrk

@n-riesco
Copy link
Owner

Unfortunately, Javascript is camelcase whereas Python's PEP 8 recommends under_scores:

  • message.parentHeader is only used in Javascript code
  • msg_id is defined by the IPython spec

This was an early decision in the project (when I was the main/only user of this module). This is not the case any longer and I would understand if others would prefer message.parent_header.msg_ig.

This is the kind of incompatible change you don't want to make when the library is mature. I think that if this naming decision is:

  • prompting bugs, and/or
  • others like message.parent_header.msg_ig better
    we should make change now.

@minrk
Copy link

minrk commented Jul 17, 2015

While I don't think it's super important, parent_header is actually part of the spec, but the zmq wire-format explodes the top-level dict into frames, relying on the receiver to reconstruct the dict. This makes parent_header the only key where the receiver even has the opportunity to deviate from the spec. This may be relevant for Javascript in particular, because Javascript code can interact with these same messages over two different wire formats in different contexts:

  • zmq messages directly
  • browser-side websocket messages

In the websocket case, since websockets don't support framing, the whole message is packed up as a single dict, and the receiver doesn't have the opportunity to reconstruct the dict with its own choice of keys. Having the parent key be different in those two contexts could get frustrating for people writing libraries that operate on messages that work in both the browser and node/electron (ahem, @rgbkrk).

parentHeader can be kept for a while as a deprecated alias, for a smoother transition. Or you could shim all the underscores with idiomatic js camelCase with get/set properties (that's probably a bit too far).

@n-riesco
Copy link
Owner

Just to be clear. I'm reading message in @rgbkrk 's issue as an instance of Jmp~Message. This means that (regardless of the naming choice) the JMP library has to call message._encode() before sending the message over the wire.

Having said this, I understand Min's argument that in websocket's case there is no choice but using under_score naming. Honestly, I now think that parentHeader was a bad choice on my side. The argument for keeping consistency with IPython (and the browser side) is stronger than the naming expectations one may have in Javascript.


About how to introduce this incompatible change, given that the library is still so young, I'd rather avoid accumulating cruft. I would suggest (as I did with the last incompatible change) that we:

  • deprecate message.parentHeader (update README.md)
  • create a branch v0.1 that keeps using message.parentHeader
  • bump the version to v0.2.0 and rename message.parentHeader as message.parent_header
  • maintain branch v0.1 for some time

@willwhitney , since you are also planning to use JMP, what are your thoughts?

@willwhitney
Copy link
Contributor

I think we don't need to worry about maintaining 0.1 - as you say, this
project is still quite young, and there probably are basically no people
that updated to 0.1 already, with its changes, and won't update to 0.2
without problem.

Breaking changes are fine with me as a user as long as they are versioned
appropriately.

Otherwise, sounds good!

On Fri, Jul 17, 2015, 3:46 AM Nicolas Riesco notifications@github.com
wrote:

Just to be clear. I'm reading message in @rgbkrk
https://github.com/rgbkrk 's issue as an instance of Jmp~Message
http://n-riesco.github.io/jmp/module-jmp-Message.html. This means that
(regardless of the naming choice) the JMP library has to call
message._encode()
http://n-riesco.github.io/jmp/module-jmp-Message.html#_encode before
sending the message over the wire.

Having said this, I understand Min's argument that in websocket's case
there is no choice but using under_score naming. Honestly, I now think that
parentHeader was a bad choice on my side. The argument for keeping
consistency with IPython (and the browser side) is stronger than the naming

expectations one may have in Javascript.

About how to introduce this incompatible change, given that the library is
still so young, I'd rather avoid accumulating cruft. I would suggest (as I
did with the last incompatible change) that we:

  • deprecate message.parentHeader (update README.md)
  • create a branch v0.1 that keeps using message.parentHeader
  • bump the version to v0.2.0 and rename message.parentHeader as
    message.parent_header
  • maintain branch v0.1 for some time

@willwhitney https://github.com/willwhitney , since you are also
planning to use JMP, what are your thoughts?


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

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Jul 17, 2015

Yup, I'll just update my code to adapt to v0.2.0 as I want to work across the websocket and direct zmq case in the same way.

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Jul 17, 2015

For a bit more transparency on use cases, we've been working on some components for inclusion on the web (thebe, the notebook (potentially), adapting some of my current demonstration code, etc.) as well as in Electron and Atom (where we'd use jmp directly). If the messages are consistent, I can work across several frontends with the same javascript.

Thanks for the discussion all! 👍

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Jul 17, 2015

Going back and reading Min's comment again, he outlined exactly what I said... so that was me confirming it. 😉

@n-riesco
Copy link
Owner

n-riesco commented Jul 17, 2015 via email

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Jul 17, 2015

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants