Spacebrew / spacebrew

A dynamic re-routable software toolkit for choreographing interactive spaces.
MIT License
222 stars 50 forks source link

Types should not be forced to be lowercase #43

Open robotconscience opened 10 years ago

robotconscience commented 10 years ago

Is there a reason behind this? Only discovered this when trying to send a custom type of "point2D", which the server crunched to "point2d".

https://github.com/Spacebrew/spacebrew/blob/master/spacebrew.js#L516 https://github.com/Spacebrew/spacebrew/blob/master/spacebrew.js#L530

Unless there's a compelling reason to keep it this way, I say we change it.

quinkennedy commented 10 years ago

The original motivation behind this was to be flexible with matching "STRING" and "string". If you want those two types to be routable, then it is simple enough to do a case-ignorant compare on the server side, but in the admin, do they both show up as defined via their client's config files? (so in the UI you see different casings for the same type?)

It was done in the name of normalization.

robotconscience commented 10 years ago

I get that. I believe it only happens on the server side, so all of the clients' config files look good, then messages just mysteriously disappear on the server. I think for the goal of flexibility with custom types it might make sense to remove it. Couple with adding constants to the libraries for the basic types (e.g. Spacebrew.String = "string") to help ease misspelling/mis-capitalization woes, I think it's a good setup.

@julioterra what do you think?

julioterra commented 10 years ago

My not-well-thought-out perspective is to remove it.

On Thu, Jan 23, 2014 at 1:20 PM, Brett Renfer notifications@github.comwrote:

I get that. I believe it only happens on the server side, so all of the clients' config files look good, then messages just mysteriously disappear on the server. I think for the goal of flexibility with custom types it might make sense to remove it. Couple with adding constants to the libraries for the basic types (e.g. Spacebrew.String = "string") to help ease misspelling/mis-capitalization woes, I think it's a good setup.

@julioterra https://github.com/julioterra what do you think?

— Reply to this email directly or view it on GitHubhttps://github.com/Spacebrew/spacebrew/issues/43#issuecomment-33152302 .

quinkennedy commented 10 years ago

Hey Brett, I would expect the server to apply a blanket .toLower() on all pub/sub types and then run .equals() between those strings, are you saying that your subscriber receives messages with type="point2d" when it is expecting "point2D" or that the server never forwards the messages at all when a publisher and subscriber are routed together with the type "point2D"?

robotconscience commented 10 years ago

The case we had is we were sending as "point2D" and receiving as "point2d".

I think it makes sense to do the toLower() for comparison, as long as it preserves the types from app to app.

quinkennedy commented 8 years ago

so if we have a route from ClientA::PublisherA::Mytype to ClientB::SubscriberB::myType. when a message is published by PublisherA, does SubscriberB receive it with type Mytype, or myType?

I suppose I would say it is the least confusing if you receive it as myType (the same capitalisation as SubscriberB subscribed with.)

robotconscience commented 8 years ago

Such a tricky one.

My gut is actually that myType != MyType. is that crazy?

quinkennedy commented 8 years ago

We have three related questions (using the convention of <Client Name>::<Endpoint Name>::<Endpoint Type>):

  1. Can both ClientA::PublisherA::STRING and ClientB::SubscriberB::String be registered with the server
  2. Can ClientA::PublisherA::STRING be routed to ClientB::SubscriberB::String
  3. When ClientB::SubscriberB receives a message published by ClientA::PublisherA, is the type in the JSON message "STRING", "String", or "string"

Currently the server answers these questions as follows:

  1. Yes
  2. Yes
  3. "string"

And it seems like Brett answers them:

  1. Yes
  2. No
  3. Moot

And Quin suggested answering them:

  1. Yes
  2. Yes
  3. "String"

I think we agree that the approach decided on should keep the system as flexible as possible, and make it obvious what the problem is when something unexpected happens.

I will attempt to argue each of our points of view. the Server PoV is a result of implementation details which were not deeply considered. Please correct me if I am mis-representing a viewpoint, and please let us know which viewpoint you agree with (or present a new viewpoint):

Brett viewpoint

People should be made aware of capitalisation inconsistencies as early as possible. By not allowing types with mis-matched capitalisation to be routed together, these common errors or misunderstandings can be fixed early.

It will help to expose this inconsistency in the Admin (which currently applies text-transform:uppercase to all type names)

Quin viewpoint

Capitalisation inconsistencies are common, especially between different authors. Even if two clients are designed to connect to one-another, it is understandable that the types may use different capitalisation. We can coerce the type to === the subscriber's registered type since within one client it should be expected that type capitalisation will be consistent.

robotconscience commented 8 years ago

To complicate things, I offer a caveat to my viewpoint: I think that built-in types should be case-insensitive. So "STRING" = "string" = "StRiNg"

Is that crazy?

quinkennedy commented 8 years ago

you have just crossed over the threshold to crazy-town

quinkennedy commented 8 years ago

I'm just concerned that making capitalisation-minded comparison only apply to custom types will make understanding capitalisation-based errors with custom types harder.

I understand that it "lowers the floor" without "lowering the ceiling", but I think it introduces a frustrating and somewhat arbitrary hurdle between basic usage and advanced usage. Hopefully it is only people writing libraries (or using a language/platform which doesn't have a library) dealing with built-in capitalisation questions.

robotconscience commented 8 years ago

I totally hear you. I think your earlier suggestion makes sense, but I do think we should still enforce the default types to conform to "string", "range", and "boolean".

So the flow could be (and I think this is what you were suggesting?):

I'm OK if we do the same for the default types, up in the air on that one. Could we also add a flag that lets the user know on the client side what happened? Like an error code or warning code?