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

Clarify that unions are allowed in JSON.mapping #5483

Merged
merged 2 commits into from Jan 2, 2018

Conversation

RX14
Copy link
Contributor

@RX14 RX14 commented Dec 30, 2017

No description provided.

@straight-shoota
Copy link
Member

For reference: question about this on SO

Besides fixing the obvious mistake it would also be great to clarify the behaviour regardin union types.

@bcardiff
Copy link
Member

It's not true that an alphabetical order is used. First the primitive ones are used https://github.com/crystal-lang/crystal/blob/master/src/json/from_json.cr#L192
and the the alphabetical of non primitives is used (but mainly as a side effect of how unions are stored)

@RX14
Copy link
Contributor Author

RX14 commented Dec 31, 2017

@bcardiff I'm half tempted to use the dreaded words "implementation-defined behaviour" to describe the order to make it open to optimization past 1.0...

@asterite
Copy link
Member

asterite commented Jan 2, 2018

I would say "implementation defined". If two models can be parsed without problem from a same JSON, then you can't rely on which one you'll get and you probably have to use some other way to parse that.

@RX14
Copy link
Contributor Author

RX14 commented Jan 2, 2018

Changed to just "undefined". It doesn't really matter to the user if it's implementation-defined or not. It's simply behaviour they cannot rely on, hence undefined.

@RX14 RX14 added this to the Next milestone Jan 2, 2018
@RX14 RX14 merged commit 6399e09 into crystal-lang:master Jan 2, 2018
lukeasrodgers pushed a commit to lukeasrodgers/crystal that referenced this pull request Jan 7, 2018
* Clarify that unions are allowed in JSON.mapping

* fixup! Clarify that unions are allowed in JSON.mapping
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants