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

Types should not be forced to be lowercase #43

Open
robotconscience opened this issue Jan 23, 2014 · 12 comments
Open

Types should not be forced to be lowercase #43

robotconscience opened this issue Jan 23, 2014 · 12 comments

Comments

@robotconscience
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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

On Thu, Jan 23, 2014 at 1:20 PM, Brett Renfer notifications@git.luolix.topwrote:

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//issues/43#issuecomment-33152302
.

@quinkennedy
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

Such a tricky one.

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

@quinkennedy
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

you have just crossed over the threshold to crazy-town

@quinkennedy
Copy link
Member

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
Copy link
Member Author

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?):

  • user sends a route on custom "coolFloat" route
  • server transforms to uppercase "COOLFLOAT" to test comparison
  • server sends on matching route – e.g. if publisher wants "CoolFloat" it gets "CoolFloat"

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?

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

No branches or pull requests

3 participants