Allow unshared instances of the manager #101

vokal-isaac · 2016-10-13T18:29:37Z

In order to allow multiple instances of VOKCoreDataManager to coexist:

any instance methods on VOKCoreDataManager should use self instead of the shared instance
any class methods that require an instance of VOKCoreDataManager should be converted to instance methods (_breaking change_)
VOKCoreDataManager should not use any methods from NSManagedObject+VOKManagedObjectAdditions that require a manager instance

The methods in NSManagedObject+VOKManagedObjectAdditions that require a manager instance are now all convenience wrappers that call instance methods on the shared manager. This means that having multiple manager instances means using the manager methods directly rather than the convenience category methods on the managed objects.

@vokal/ios-developers @chillpop Code review please?

…mplementation. This converts two class methods into instance methods, so is a breaking change.

…terface.

…ance.

chillpop · 2016-10-13T18:42:58Z

Is the idea to allow multiple instances in addition to the sharedInstance, or deprecate the sharedInstance entirely?

vokal-isaac · 2016-10-13T18:49:40Z

In addition to sharedInstance. I think the vast majority of the time, sharedInstance is the way to go. I just happen to be looking at a particular use case on macOS where the data for a document is represented with an in-memory Core Data store and multiple documents can be open at once. I think having a manager per document is cleaner than designing the Core Data models to allow multiple documents' models to coexist in a single store.

chillpop · 2016-10-13T18:54:09Z

Pod/Classes/NSManagedObject+VOKManagedObjectAdditions.m

-    }
-
-    return vok_entityName;
+    return [[VOKCoreDataManager sharedInstance] entityNameForClass:self];


Given that the idea is to use instances at the same time as the sharedInstance, I think this method (vok_entityName) should be deprecated in favor of using the instance method on VOKCoreDataManager. If your managed object is not in the shared instance's object model, you won't get the right value here. Eg: it's in a different MOM on a separate Vokoder instance.

My inclination was to leave it in place in the same spirit as the rest of the methods in the category—they're fine if you're only using sharedInstance and they're totally wrong/broken if you're not.

That expectation should be prominently documented in the header for this category.

chillpop · 2016-10-13T19:02:57Z

Pod/Classes/NSManagedObject+VOKManagedObjectAdditions.m

-                                         }
-                                     }];
+    VOKCoreDataManager *coreDataManager = VOKCoreDataManager.sharedInstance;
+    [coreDataManager importArrayInBackground:inputArray


What's the plan for using this and other convenience methods with a non-shared VOKCoreDataManager? You can't?

Right. Basically, the convenience methods in NSManagedObject+VOKManagedObjectAdditions are (now that vok_entityName has been rewritten) just wrappers around methods on the manager, so if one needs a manager-specific call, passing the manager in as an argument to a method on the managed object seemed uglier than calling the method on the manager with the managed object as an argument.

There probably should be an explicit note to the effect of "things in NSManagedObject+VOKManagedObjectAdditions use VOKCoreDataManager.sharedInstance."

Sound good.

chillpop · 2016-10-13T19:08:42Z

Pod/Classes/Optional Data Sources/VOKFetchedResultsDataSource.h

+                              cacheName:(NSString *)cacheName
+                              tableView:(UITableView *)tableView
+                     sectionNameKeyPath:(NSString *)sectionNameKeyPath
+                        sortDescriptors:(NSArray *)sortDescriptors


I believe all the parameters from here and above should be marked nullable.
Also: this one should be VOKArrayOfSortDescriptors to get a typed array

Whoops. I really wish Xcode's autocomplete on the .m side would carry over the nullability annotations, etc., from the .h side. (This was copied back from the .m, hence missing all that stuff.)

chillpop · 2016-10-13T19:09:56Z

Pod/Classes/Optional Data Sources/VOKFetchedResultsDataSource.h

+                     managedObjectClass:(Class)managedObjectClass
+                              batchSize:(NSInteger)batchSize
+                             fetchLimit:(NSInteger)fetchLimit
+                               delegate:(id <VOKFetchedResultsDataSourceDelegate>)delegate;


nullable here too

chillpop · 2016-10-13T19:12:24Z

Pod/Classes/VOKCoreDataManager.m

@@ -97,7 +99,7 @@ - (void)setResource:(nullable NSString *)resource
    }

    // Touch the managed object context to ensure it's been created
-    [[VOKCoreDataManager sharedInstance] managedObjectContext];
+    [self managedObjectContext];


self.managedObjectContext?

I dislike the dot notation when we're using it for a side-effect, which feels off anyway... but maybe (void)self.managedObjectContext;, to acknowledge that we're not using the retrieved value?

Enh, keep it as is. I wasn't paying attention to the context of the call.

brockboland

Is there a good way to unit-test multiple VOKCoreDataManagers?

This change also merits some additional documentation. Could you add something brief to the README, just noting that multiple instances are supported?

brockboland · 2016-10-13T21:28:54Z

Pod/Classes/NSManagedObject+VOKManagedObjectAdditions.m

-    }
-
-    return vok_entityName;
+    return [[VOKCoreDataManager sharedInstance] entityNameForClass:self];


That expectation should be prominently documented in the header for this category.

brockboland · 2016-10-13T21:31:11Z

Pod/Classes/Optional Data Sources/VOKFetchedResultsDataSource.m

+                       fetchLimit:(NSInteger)fetchLimit
+                         delegate:(id <VOKFetchedResultsDataSourceDelegate>)delegate
+{
+    return [self initWithCoreDataManager:VOKCoreDataManager.sharedInstance


I would pass nil here to let the designated initializer handle it

brockboland · 2016-10-13T21:33:36Z

Pod/Classes/VOKCoreDataManager.h

@@ -302,7 +309,7 @@ typedef void(^VOKObjectIDsReturnBlock)(VOKArrayOfManagedObjectIDs *managedObject
 @param objectClass     Specifies the class to instantiate or fetch when importing data.
 @param completion      Fired on the main queue once the changes have been merged. It brings an NSArray of permanent NSManagedObjectIDs matching the objects deserialized from the import array.
 */
-+ (void)importArrayInBackground:(VOKArrayOfObjectDictionaries *)inputArray
+- (void)importArrayInBackground:(VOKArrayOfObjectDictionaries *)inputArray


I can't find it now, but I thought there was a compiler macro to indicate that some method has been replaced with another one, so that Xcode can suggest the appropriate correction. I can't find it now, but if that rings any bells, it might be helpful to include it here.

…here and for writeToTemporaryContext above, that is.

If I left the old methods in place, they could be tagged with the deprecation macro with a message pointing to the new methods.

I'd say maybe we add the instance methods to 3.x and mark the class methods as deprecated there, with the note that they'll be removed in 4.x?

We discussed this in person, but: nah, I'd rather just move forward with the changes in 4.x and leave 3.x where it is.

brockboland · 2016-10-13T21:36:44Z

Pod/Classes/VOKCoreDataManager.m

@@ -8,6 +8,8 @@
 #import "VOKCoreDataManager.h"
 #import "VOKCoreDataManagerInternalMacros.h"

+#import <objc/runtime.h>


Looks like this import can be removed from NSManagedObject+VOKManagedObjectAdditions.m. I can't comment on the line over there because it's not part of the diff.

brockboland · 2016-10-13T21:39:33Z

Pod/Classes/VOKCoreDataManager.m

@@ -631,36 +682,38 @@ - (void)saveAndMergeWithMainContextAndWait:(NSManagedObjectContext *)context;

 #pragma mark - Background Importing

-+ (void)writeToTemporaryContext:(VOKWriteBlock)writeBlock
+- (void)writeToTemporaryContext:(VOKWriteBlock)writeBlock


Same thing in a couple places here, regarding that compiler macro for replaced methods (if we find it)

…hared instance.

brockboland · 2016-10-17T17:07:24Z

@chillpop @bryanluby any other feedback on this one?

chillpop · 2016-10-17T17:15:41Z

still LGTM

bryanluby · 2016-10-17T18:48:36Z

LGTM

vokal-isaac added 5 commits October 13, 2016 11:06

Tweak singleton pattern.

4bb88c3

Avoid referencing the shared instance within the VOKCoreDataManager i…

a437676

…mplementation. This converts two class methods into instance methods, so is a breaking change.

Update the sample projects to match the revised VOKCoreDataManager in…

ebce419

…terface.

Decouple the fetched results data source from the shared manager inst…

f8cf0e4

…ance.

Decouple the entityName method from the shared manager instance.

95c451b

chillpop reviewed Oct 13, 2016

View reviewed changes

Add missing nullability annotations.

1159822

chillpop approved these changes Oct 13, 2016

View reviewed changes

brockboland suggested changes Oct 13, 2016

View reviewed changes

vokal-isaac added 3 commits October 14, 2016 14:05

Add comment to NSManagedObject category noting that it's all on the s…

a6d322c

…hared instance.

Document & use the nil-manager behavior of the data source initializer.

1537bba

Remove unnecessary import.

b295395

brockboland approved these changes Oct 17, 2016

View reviewed changes

brockboland merged commit 0b905dc into vokal:master Oct 17, 2016

vokal-isaac deleted the feature/allowUnsharedInstances branch November 4, 2016 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow unshared instances of the manager #101

Allow unshared instances of the manager #101

vokal-isaac commented Oct 13, 2016

chillpop commented Oct 13, 2016

vokal-isaac commented Oct 13, 2016

chillpop Oct 13, 2016

vokal-isaac Oct 13, 2016

brockboland Oct 13, 2016

chillpop Oct 13, 2016

vokal-isaac Oct 13, 2016

chillpop Oct 13, 2016

chillpop Oct 13, 2016

vokal-isaac Oct 13, 2016

chillpop Oct 13, 2016

chillpop Oct 13, 2016

vokal-isaac Oct 13, 2016

chillpop Oct 13, 2016

brockboland left a comment

brockboland Oct 13, 2016

brockboland Oct 13, 2016

brockboland Oct 13, 2016

brockboland Oct 13, 2016

vokal-isaac Oct 14, 2016

brockboland Oct 17, 2016

brockboland Oct 13, 2016

brockboland Oct 13, 2016

brockboland commented Oct 17, 2016

chillpop commented Oct 17, 2016

bryanluby commented Oct 17, 2016

Allow unshared instances of the manager #101

Allow unshared instances of the manager #101

Conversation

vokal-isaac commented Oct 13, 2016

chillpop commented Oct 13, 2016

vokal-isaac commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brockboland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brockboland commented Oct 17, 2016

chillpop commented Oct 17, 2016

bryanluby commented Oct 17, 2016