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

Add JSONDeserializer for transforming NSData to JSON #70

Closed
wants to merge 3 commits into from

Conversation

gfontenot
Copy link
Collaborator

This adds a failable initializer that wraps up the NSData -> AnyObject -> JSON transformation into a self contained package. Now all you need
to do is pass the NSData directly to JSON and you'll get an optional
JSON value back. That value can then be (conditionally) passed into a
decoder for parsing.

Open to comments here. Should this live somewhere other than this?

Fixes #68

@tonyd256
Copy link
Contributor

I can see an argument for it not living here because technically Argo could decode anything that comes in a Swift/Objc object form.

In the future, if we look into changing the JSON enum name I would probably change the init to something like public init?(jsonData: NSData) so we could have other basic initializers.

I am ok with this though.

@gfontenot
Copy link
Collaborator Author

That's a good point. What about throwing this on a simple JSONDeserialization class that only does this?

@tonyd256
Copy link
Contributor

Ya I think I like the idea of it being separate from this JSON enum ... I would be more into that

@gfontenot gfontenot changed the title Add failable initializer for parsing NSData Add JSONDeserializer for transforming NSData to JSON Feb 25, 2015
@gfontenot
Copy link
Collaborator Author

@tonyd256 Want to take another look? Also ping @thoughtbot/ios for thoughts on this?

@tonyd256
Copy link
Contributor

I think this is better

@gfontenot gfontenot added this to the 1.0.0 - No More Excuses milestone Feb 25, 2015
import Runes

public struct JSONDeserializer {
public static func deserialize(data: NSData) -> JSON? {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too early to yank this up to a Deserializer protocol?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably. I think that's an easy enough refactor later on if we actually have real-world usage for parsing other sources of NSData

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair. My thinking here is that it would at least indicate to consumers of this framework that they can implement additional deserializable types if they want to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that we shouldn't be obscuring the ability to set reading options here. Consider a response that doesn't begin with an array or object. For instance...

response_date: 2015-03-04T12:43:20Z,
post: {
    id: 1
    title: "JSON Fragments Considered Harmful"
}

Attempting to parse this with NSJSONSerialization without specifying the NSJSONReadingOptionsAllowFragments option would return an error: The operation couldn’t be completed. (Cocoa error 3840.)" (JSON text did not start with array or object and option to allow fragments not set.)

To work around this, I suggest we overload deserialize(data:) -> JSON? with deserialize(data:allowFragments:) -> JSON?.

I'm pretty sure we can disregard the other reading options, .MutableContainers and .MutableLeaves but I could be convinced that we should add more overloads or a function that just takes an NSJSONReadingOptions if there's a really good use case for it.

@gfontenot
Copy link
Collaborator Author

@tonyd256 What do we think about this idea now that we have Decoded<T> and the global decode functions?

This adds a failable initializer that wraps up the `NSData -> AnyObject
-> JSON` transformation into a self contained package. Now all you need
to do is pass the `NSData` directly to `JSON` and you'll get an optional
`JSON` value back. That value can then be (conditionally) passed into a
decoder for parsing.
This encapsulates the transformation of `NSData` through
`NSJSONSerialization` into a `JSON` object in a `JSONDeserializer`
struct. The only responsibility of this object is this transformation of
data.
@gfontenot
Copy link
Collaborator Author

After discussing with @tonyd256 in meat space, we don't think this is super useful. Micromanaging the NSData deserialization doesn't seem worth it, and it puts us in a weird place where we have NSData -> JSON (skipping AnyObject), and AnyObject -> Decoded<T> (skipping JSON). Including JSON specific decode functions just to avoid NSJSONSerialization feels like over architecting. Instead, we want to improve Decoded so that it's easier to simply flatMap the AnyObject? value from NSJSONSerialization and get back T? or Decoded<T>

I've taken a stab at this in #96 and am closing this PR in favor of that one.

@gfontenot gfontenot closed this Mar 27, 2015
@gfontenot gfontenot removed the wip label Mar 27, 2015
@gfontenot gfontenot deleted the gf-nsjsonserialization branch May 4, 2015 13:33
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.

Wrap up the NSData -> JSON process in something
3 participants