-shouldImport<relationshipName>: not called if -import<relationshipName>: returns YES

[Alydyn]

Per @casademora's article on CIMGF:

shouldImport<;relationshipName>;: Sometimes, you’re data for importing is good, except for a small subset of data connected via a relationship. Rather than going through and wasting precious cycles importing the data, only to toss it later when you determine it’s not useful, MagicalRecord provides a nice callback basically asking permission to import a relationship. At this point, you can reach down into the data, perform your check, and return YES if you should import that set of data, or NO if you don’t want that data to be imported. If no shouldImport<;relationshipName>;: instance method is defined on your entity, MagicalRecord will assume you want all that data below. That makes this a deviation on the standard importing logic, when you need it.

• (BOOL) shouldImportContacts:(id)data; { NSString *zipCode = [[data lastObject] valueForKey:@"zipCode"]; return IsFormattedAsZipPlusFour(zipCode); } In this example, we’re checking the last object in our list for a zip code with the zip+4 format. When it’s not valid, we’ll assume the whole lump of data should be skipped. This code snippet is provided as an example of how to author a shouldImport<;relationshipName>;: method.

As I understand it, the implementation of -shouldImport<relationshipName> is to be used as a quick check to validate incoming data. If not implemented, or if implemented and returns YES, then -import<relationshipName>: should be called.

As it is implemented in 2.3, -import<relationshipName>: is called first. If this returns YES then the for-in loop continues, and -shouldImport<relationshipName>: is never called. This effectively means that the "quick check" needs to be implemented in the -import<relationshipName>: method as well, and is at that point superfluous.

@tonyarnold or @casademora if I'm correct, I can submit a pull request with the necessary change if you would like.

[tonyarnold]

Hrm, your description sounds logical. Yes, please, if you have time go ahead and work up a PR for this. If there are tests for this, could you please also update them? Thanks!

[Alydyn]

@tonyarnold I'll start on this. Also another question for you: The -shouldImport<>: and -import<>: methods each pass in different data. Is this by design? For example, suppose I have an Author entity and Book entity. I am importing an author and get to the books relationship. -shouldImportBooks: passes in only the data for that relationship, such as:

["bookID1",
"bookID2",
"bookID3"]

However the importBooks: method passes in all of the data about the Author, such as:

{
"name" : "John Doe",
"Publisher" : "Random House",
"books" : ["bookID1",
"bookID2",
"bookID3"]
}

Looking at the code, this is because -shouldImport is handed relatedObjectData, but the import method is called with relationshipData. Looking back to @casademora 's cimgf.com article it would appear that data is assumed to only contain information about the relationship:

- (BOOL) importKeywords:(id)data;
{
    NSCharacterSet *set = [NSCharacterSet characterSetWithCharactersInString:@" ,"];
    self.keywords = [data componentsSeparatedByCharactersInSet:set];
    return YES;
}

This suggests to me that -import<>: should be called with the same data as the -shouldImport<> method, which is relatedObjectData in code.

Thoughts?

[tonyarnold]

My only worry with changing this behaviour now, is that 2.3 is a minor release — it shouldn't drastically alter the way something complicated like data import works. That being said, I don't fully understand why @casademora chose to architect this method in the way you describe. There may be performance or other considerations here that I'm not seeing.

Why don't you keep the data being passed the same for now — at least until you or I understand what flow on effects might come from the change you describe.

[Alydyn]

@tonyarnold I understand, that sounds fair. Thanks!

[tonyarnold]

Sorry to push, @Alydyn — I'd like to release 2.3.0 this week — any chance your changes to this PR will be ready?

[Alydyn]

As soon as I saw you posting again it reminded me of this! Yes, I have the changes completed, but I do not know how to write the tests that you use. I'll send it tomorrow as working on getting kids to bed now.

Sent from my iPhone

On Nov 24, 2014, at 5:58 PM, Tony Arnold notifications@github.com wrote:

Sorry to push, @Alydyn — I'd like to release 2.3.0 this week — any chance your changes to this PR will be ready?

— Reply to this email directly or view it on GitHub.

[tonyarnold]

Not a problem @Alydyn — the tests are all in XCTest, using Expecta for the actual expectations within each test case. What testing framework are you familiar with? I'm happy to work with you to get this bit right.

[Alydyn]

@tonyarnold I'm embarrassed to say I haven't used any testing frameworks. I have looked over what is included in MR though and if you can give me some quick pointers, I'm confident I can write a test for this fix.

[tonyarnold]

That's completely OK. If you have a look at the existing tests, they're broken down into logical areas that need testing, then into individual - (void)testSomething methods that represent small blocks (or units) of functionality. Within the method, you test expectations of the state of whatever you're doing.

The documentation for Expecta is at https://github.com/specta/expecta, and we use Apple's XCTest which is well documented in Xcode's documentation browser.

In your shoes, I'd be looking for tests that do something close to what I want and copying them — don't be too worried, I'll review whatever you do and give you feedback if I think there's anything you've not quite covered. It's pretty much an essential skill for app developers now, so feel free to ask any questions — I'm not an expert by any means, but I'm happy to share what I do know.

[Alydyn]

@tonyarnold sorry for the delay... I am hoping to work on this today and have something for you. Thanks for your help!

[Alydyn]

@tonyarnold I'd like to run by you what I think I need to do and then go ahead and implement once I have it correct. First, I couldn't find any tests for the various other import checks, such as -shouldImport, -willImport, etc. If I'm not missing them somewhere I can work on those later. Also I don't think Expecta is used in the import tests... can you see if I'm missing something there?

Okay, so I think that first I need to add a couple of new entities (ShouldImportEntity and RelatedShouldImportEntity?), as I will need to add the -import<XXX> methods. Then create a relationship between them and give the RelatedShouldImportEntity a property, probably an objectId type of property. It looks like Mogenerator was used to created the NSManagedObject subclasses but I don't see it set up, so I'll quick do that and run it. Then I'll need to create some test JSON data.

In the subclasses I'll implement -shouldImportRelatedEntity:(id)data, and -importRelatedEntity:(id)data and return YES or NO depending on the values as structured by the test data that I have created.

Then I subclass MagicalRecordDataImportTestCase', creating only the .m file (ShouldImportRelationshipsTests.m?). In this file I will override-setUpand-tearDown`, and also write my test methods.

How does that sound?

[tonyarnold]

• You don't need to use Expecta — I think I shied away from it for a while when working on MR's tests.
• Yes, add the new entities you need for your tests to the test data model. Don't add mogenerator to the project — just use it to generate the subclasses you need. You shouldn't include external tools in your build chain not absolutely necessary.

Otherwise, that sounds good. Thanks for being willing to have a crack at this.

[tonyarnold]

@Alydyn any chance you could push up the PR for this, tests or no?

[tonyarnold]

Pushing off to MagicalRecord 3.0 so that I can get 2.3.0 out the door.

[Alydyn]

Hey @tonyarnold I am really really sorry about that. I completely missed the email with your question.

This is a good idea... 2.3 really needed to get release!

Thanks for your hard work on MR

[tonyarnold]

No stress at all @Alydyn :smile: