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 Serialization to JSON String and Cocoa Touch Native Objects #698

Closed
wants to merge 14 commits into from

Conversation

c0rrzin
Copy link

@c0rrzin c0rrzin commented Jul 28, 2014

references #694

TODO:

  • Change Cocoa-touch serializers' method signatures
  • Add test cases for circular references
  • Use RLMDynamicGet() instead of performing primitive selectors

@@ -188,11 +215,121 @@ +(RLMArray *)objectsInRealm:(RLMRealm *)realm withPredicate:(NSPredicate *)predi
return RLMGetObjects(realm, self.className, predicate, nil);
}

- (NSDictionary *)NSDictionary {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to call this method JSONDictionary

@alazier
Copy link
Contributor

alazier commented Jul 28, 2014

Good start. We should definitely be using `RLMDyamicGet`` instead of calling primitive selectors.

@c0rrzin
Copy link
Author

c0rrzin commented Jul 29, 2014

I updated the PR's description so that everyone can keep track of what's missing.

@c0rrzin
Copy link
Author

c0rrzin commented Jul 31, 2014

@alazier I don't have access to the CI, but I think the build is not passing due to the unpredictability of [NSJSONSerialization dataWithJSONObject:options:error] parsing order. Do you have any idea how can this problem be solved? (I know that deep down we are testing the API, but I think it's nice to have the strings tested too)

Thanks.

@alazier
Copy link
Contributor

alazier commented Aug 1, 2014

The two failing test conditions are:

x testAllKindSerialization, ((objTestDictionary) equal to (objDictionary)) failed: ("{
x testAllKindSerialization, ((objTestString) equal to (objString)) failed: ("{

I'm not sure which objects don't align for the first case - my guess would be either the date or the binary data. For the second case, we could simply parse the strings using NSJSONSerialization and compare the dictionaries. I don't think there is a reliable way to compare the strings directly.

@tgoyne
Copy link
Member

tgoyne commented Aug 1, 2014

Both tests are in fact failing due to dictionary keys not being in a consistent order. I agree it'd be nice to test the exact string to verify that non-semantic things like formatting are correct, but I don't think that's possible for anything involving multiple items in a dictionary.

@c0rrzin
Copy link
Author

c0rrzin commented Aug 1, 2014

@tgoyne I've been researching and did not found a simple way for doing that as well. Maybe I should test every valid property type and then maybe also test the count of properties after serializing. Will it be enough?

#pragma mark -

/**---------------------------------------------------------------------------------------
* @name Serializing an Arrat to NSArray
Copy link
Contributor

Choose a reason for hiding this comment

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

Arrat -> Array

@@ -220,9 +220,25 @@ - (NSUInteger)indexOfObjectWithPredicate:(NSPredicate *)predicate
}
#pragma GCC diagnostic pop

- (NSArray *)JSONArray {
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation must be 4 spaces (allover)

@bmunkholm
Copy link
Contributor

Need to update /CHANGELOG.md as well.

@bmunkholm
Copy link
Contributor

We already got a version of similar serialization in core that we should consider using to centralize the functionality if possible.

@c0rrzin
Copy link
Author

c0rrzin commented Aug 20, 2014

@bmunkholm how should we proceed then? Is the API accessible via tightdb headers?

@alazier
Copy link
Contributor

alazier commented Aug 20, 2014

@bmunkholm @c0rrzin the core apis are not suitable for everything we need to do in the objc api. For instance we need to support bi-directional keyPath mapping, custom date formatters, and perhaps at some point arbitrary transformations. As discussed previously it makes sense to get this working first in the objc api and then provide the needed functionality in core so this functionality can be shared across all apis.

@c0rrzin
Copy link
Author

c0rrzin commented Aug 20, 2014

I'm thinking if this serializer isn't complex enough to be encapsulated. @alazier @bmunkholm ?

@alazier
Copy link
Contributor

alazier commented Aug 20, 2014

I'm not sure yet. I think what we want to do next is put together a full list of requirements, and come up with an api which best addresses those requirements. Will hopefully be able to get to this sometime this week.

As far as having multiple classes, at this point I'm envisioning we have at least on other class which allows users to specify JSON mappings, formatters, and other transformation. This could potentially be passed into the method used to generate JSON, or perhaps you could have a mapper that is used by default. It won't be possible to do this well though until we have the full list of features/requirements we hope to support so defining those should definitely be the next step.

@c0rrzin
Copy link
Author

c0rrzin commented Aug 20, 2014

@alazier agreed. If you need any help while writing this list, I can provide an initial template or something else. I think it's valid taking a look at this framework to see how they did it.

@icanzilb
Copy link
Contributor

"they" is basically me. ping me if you need some help I've been developing actively JSONModel for about 2 years

@jpsim
Copy link
Contributor

jpsim commented Dec 2, 2014

retest this please (please ignore, this is just for CI)

@segiddins
Copy link
Contributor

@c0rrzin we're unfortunately unable to merge this pull request.
For a feature as key as JSON/object mapping, we need to first map out the desired API specification before implementation, from the top down. We'll let you know when we have a pull request with our designs, and we welcome your feedback on it.
Thanks so much for your contribution to Realm, we really appreciate it when people take the time to contribute.

@segiddins segiddins closed this Jan 16, 2015
@c0rrzin
Copy link
Author

c0rrzin commented Feb 11, 2015

@segiddins I really enjoyed spending some time with the project and sure would like to effectively contribute and provide feedback on the design that Realm's team will come up with so it suits its needs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants