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

Supports subclassing Entities to an arbitrary depth, and on relationship entities #52

Merged
merged 3 commits into from
Apr 3, 2014

Conversation

iandundas
Copy link
Contributor

nb: first of all, apologies for the white-space deletions, my IDE cleaned them up. I can resubmit without, if that's much easier.

I extended the extent to which MMRecord detects and uses Entity subclasses ( i.e. when it queries shouldUseSubEntityRecordClassToRepresentData). It now supports beyond a single level of subclassing (i.e. it checks recursively), and also allows relationship entities to be matched to a subclass as well, as in this example:

A: it will now support Account subclassing for the following example:

{
    "client": {
        "portfolios":{
            "accounts":[
                {
                    "account_id": 123,
                    "account_name": "Joe Bloggs",
                    "account_type": "current-account"
                },
                {
                    "account_id": 456,
                    "account_name": "Joe Bloggs",
                    "account_type": "savings-account"
                }
            ]
        }
    }
}

Feedback is very welcome

find the most specific subclass to use. Also now supports subclasses on
an entity with a relationship to the primary entity, e.g. en
"[Tree]-<[Apples]", each Apple subclass is (recursively) queried to determine what
type of Apple it is.
@cnstoll
Copy link
Contributor

cnstoll commented Apr 1, 2014

This is looking pretty good. I want to test it out a bit more. It did pass all of my private tests related to other projects, but I did notice a small problem in another project which DOES actually use sub entities. I want to dig into that problem a bit more before this gets merged. I think that issue may be unrelated, but I want to make sure.

For now I'm gonna make a few general style comments that you can address while I look into that some more.

Thanks!

@@ -68,6 +68,7 @@ @interface MMRecordResponse ()
@property (nonatomic, copy) NSArray *responseObjectArray;
@property (nonatomic, strong) NSMutableArray *objectGraph; // Array of Protos
@property (nonatomic, strong) NSMutableDictionary *responseGroups; // Key = NSEntityDescription, Value = MMRecordResponseGroup
- (NSEntityDescription *)mostSpecificEntityForResponseObject:(id)object withInitialEntity:(NSEntityDescription *)initialEntity;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just remove this line. Its part of the private interface so we don't need to add it to the class extension here.

@iandundas
Copy link
Contributor Author

Thanks for feedback, I'll make the changes now :)

@cnstoll
Copy link
Contributor

cnstoll commented Apr 1, 2014

I tested this out again with a bit more rigor. I can't reproduce the issue I was seeing, and I think it was most likely related to a bad build. I specifically looked at a project that was using a 1-level depth of sub entities and that is still behaving correctly. I think this looks good to go once the style comments are addressed.

Thanks again!

@iandundas
Copy link
Contributor Author

Cool, I've pushed those changes now. Hope it helps! :)

cnstoll added a commit that referenced this pull request Apr 3, 2014
Supports subclassing Entities to an arbitrary depth, and on relationship entities
@cnstoll cnstoll merged commit e490d5e into mutualmobile:master Apr 3, 2014
@cnstoll
Copy link
Contributor

cnstoll commented Apr 3, 2014

There were 1 or 2 white spacing things, but I just fixed them manually. Didn't feel like thrashing on those :)

Thanks again for the PR!

@cnstoll cnstoll added this to the 1.3.1 milestone Apr 3, 2014
@cnstoll cnstoll mentioned this pull request Apr 3, 2014
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.

None yet

2 participants