Skip to content

Conversation

@csantero
Copy link
Collaborator

@csantero csantero commented Feb 2, 2015

Closes #29

This PR ensures that we support nullable versions of all the primitive types, as well as Decimal and `Decimal?. I chose to only support serializing it as a string for now.

In writing the tests for this I discovered that DateTimeOffset was not being deserialized properly. I think it's due to a bug in Json.NET, but I was able to work around it by special-casing the type. Also turns out that nullable enum deserialization was failing too. Hooray for tests.

I changed the language around "Primitives" to refer to "Attributes". The spec uses this term to refer to properties of a resource that aren't relationships, and I figured we should too.

@SphtKr
Copy link
Collaborator

SphtKr commented Feb 3, 2015

This PR ensures that we support nullable versions of all the primitive types, as well as Decimal and Decimal. I chose to only support serializing it as a string for now.

Excellent, I think this is the way to go at least now if not permanently. In a JavaScript client a conversion to number is likely to happen automatically (even accidentally) if the user wants it anyhow, so this shouldn't be a big deal.

In writing the tests for this I discovered that DateTimeOffset was not being deserialized properly.

That reminds me of a related issue I've had, I opened #45, though we may not do it at all.

I changed the language around "Primitives" to refer to "Attributes". The spec uses this term to refer to properties of a resource that aren't relationships, and I figured we should too.

Ugh...this rubs me the wrong way just because .NET "Attributes" are something completely different. However, I'm not sure that CanWriteAsPrimitive/CanWriteAsAttribute should be public (I may have done that only to facilitate testing, which of course there are better ways to do), and if it's not part of the API I don't care as much about the name. Any thoughts on changing that to private?

@SphtKr
Copy link
Collaborator

SphtKr commented Feb 3, 2015

And sorry, my merge broke your PR, can you resolve?

@csantero csantero force-pushed the more-types branch 2 times, most recently from d1aa8c8 to 8d8b82f Compare February 3, 2015 13:19
@csantero
Copy link
Collaborator Author

csantero commented Feb 3, 2015

Ugh...this rubs me the wrong way just because .NET "Attributes" are something completely different. However, I'm not sure that CanWriteAsPrimitive/CanWriteAsAttribute should be public (I may have done that only to facilitate testing, which of course there are better ways to do), and if it's not part of the API I don't care as much about the name. Any thoughts on changing that to private?

I've changed it to being an internal extension method, and renamed it to CanWriteAsJsonApiAttribute to make a more explicit distinction. I just don't like CanWriteAsPrimitive because that's really not what it is checking for.

@csantero
Copy link
Collaborator Author

csantero commented Feb 3, 2015

Pushed new diff.

SphtKr added a commit that referenced this pull request Feb 3, 2015
@SphtKr SphtKr merged commit 1f8f94c into JSONAPIdotNET:master Feb 3, 2015
@csantero csantero deleted the more-types branch February 3, 2015 15:04
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

Successfully merging this pull request may close these issues.

Support decimal type

2 participants