3Squared / SQKDataKit

Lightweight Core Data helper to reduce boilerplate code.
MIT License
19 stars 6 forks source link

SQKCoreDataOperation should only save and merge the private context it created. #6

Closed lukestringer90 closed 9 years ago

lukestringer90 commented 10 years ago

When completeOperationBySavingContext: is called the SQKCoreDataOperation should check that the specified context is the same context as was created during the start method.

A possible solution:

- (void)completeOperationBySavingContext:(NSManagedObjectContext *)managedObjectContext {
    if ([self isCancelled]) {
        [self finishOperation];
    }
        // Only save and merge the MOC that was made by this operation to start with
    else if (self.managedObjectContextToMerge == managedObjectContext) {
        [[NSNotificationCenter defaultCenter] addObserver:self
                                                 selector:@selector(contextSaveNotificationReceived:)
                                                     name:NSManagedObjectContextDidSaveNotification
                                                   object:nil];

        NSError *error = nil;
        [managedObjectContext save:&error];
        if (error) {
            NSLog(@"%@", error);
        }
    }
    else {
        [self finishOperation]
    }
}
blork commented 10 years ago

This would make it unnecessary to pass the managedObjectContext as a parameter - just use self. managedObjectContext.

blork commented 9 years ago

Isn't this accomplished in contextSaveNotificationReceived instead? At this line: https://github.com/3squared/SQKDataKit/blob/a820817dbb66165d14aa71dc0a57d21b6a326eef/Classes/shared/SQKCoreDataOperation/SQKCoreDataOperation.m#L137

it checks that managedObjectContext == self.managedObjectContextToMerge - doesn't that enforce this behaviour?

lukestringer90 commented 9 years ago

Well spotted, it looks like this is checked else where so this is not necessary.

To address your previous comment regarding passing the managedObjectContext as being unnecessary: I agree and I think then method name should be changed. I'll close this issue and open a new one to discuss further.