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

Extract the MTLModel protocol #219

Merged
merged 14 commits into from Mar 14, 2014
Merged

Extract the MTLModel protocol #219

merged 14 commits into from Mar 14, 2014

Conversation

@robb
Copy link
Member

@robb robb commented Jan 22, 2014

This extracts the core interface of MTLModel into a protocol of the same name and changes the adapters to use that instead.

This gives users of the framework a greater flexibility when integrating with existing code bases where injecting MTLModel into the inheritance chain may not be an option or when MTLModel should not be exposed as a super class. (See for example #156). For existing users, it's business as usual.

Since having a class and a protocol of the same name may be confusing, I'm all for other names for the protocol or even considering to change the name of the MTLModel class to break everybody's build (See #167).

This is just a proof of concept. Some of the documentation in the protocol still refers to implementation details of the MTLModel class implementation and should be updated accordingly.

@jspahrsummers
Copy link
Member

@jspahrsummers jspahrsummers commented Jan 23, 2014

I'm not opposed to this idea, but how would we do things like "default implementations" in this design? What happens to methods like +propertyKeys, -dictionaryValue, and our default <NSCoding> behaviors?

//
// This property must never be nil.
@property (nonatomic, copy, readonly) NSDictionary *dictionaryValue;

// Merges the value of the given key on the receiver with the value of the same
// key from the given model object, giving precedence to the other model object.
//
// By default, this method looks for a `-merge<Key>FromModel:` method on the

This comment has been minimized.

@robb

robb Jan 23, 2014
Author Member

This is more of an implementation detail slash convenience of MTLModel the class and should go there accordingly.

@robb
Copy link
Member Author

@robb robb commented Jan 23, 2014

It would be the responsibility of the respective class to fulfil the contract of +propertyKeys and -dictionaryValue. I am not sure if the rest of Mantle actually makes use of the assumption that NSCoding is implemented, so that may or may not be optional.

In the near term, I see this mostly as a way to make Mantle more adaptable to existing inheritance chains where you'd like to use MTLJSONSerializing or mix-and-match with MTLModel subclasses. I recently subclassed SKSpriteNode and friends–which already implement NSCoding– and something like this would've been quite handy.

I think the fact that this proof of concept was such an easy search-and-replace operation goes to show that there is good encapsulation in place, codifying this could help maintaining that in the future.

Having both MTLModel the protocol and MTLModel the class seems to get hairy quite easily. What's your thought on naming the class MTLObject with regards to #167?

// This is the designated initializer for this class.
- (instancetype)init;
// The default implementation combines the values corresponding to all
// +propertyKeys into a dictionary, with any nil values represented by NSNull.

This comment has been minimized.

@robb

robb Jan 23, 2014
Author Member

This probably should be reworded so that it becomes part of the contract that nil values should map to NSNull.

@jspahrsummers
Copy link
Member

@jspahrsummers jspahrsummers commented Jan 31, 2014

Why does the class need to be renamed to fulfill #167? Naming the class and the protocol MTLModel would make a lot of sense to me, if we can accomplish it.

@robb
Copy link
Member Author

@robb robb commented Jan 31, 2014

It doesn't need to, but it would certainly break everybody's build 🚎
Since there is already some confusion how MTLModel doesn't handle JSON by itself, I'd rather call them something different. Calling the protocol something else would work just as well for me, but I don't have an idea right now.

@jspahrsummers
Copy link
Member

@jspahrsummers jspahrsummers commented Feb 3, 2014

It doesn't need to, but it would certainly break everybody's build

Sorry for being obtuse, but I don't fully understand the ways in which it would break builds. Don't we have the same methods implemented on MTLModel the class in the end (even if they're declared in the protocol)?

@robb
Copy link
Member Author

@robb robb commented Feb 3, 2014

We could change the name of the class from MTLModel to e.g. MTLObject, that would break every

@interface XYModel : MTLModel
//
@end
@jspahrsummers
Copy link
Member

@jspahrsummers jspahrsummers commented Feb 3, 2014

Right, so my question was: if the class name stays the same—and some methods get declared in a protocol instead—in what way would builds break? Why would it affect existing code?

@robb
Copy link
Member Author

@robb robb commented Feb 3, 2014

Ah, sorry, misread what you said.

If we stick with MTLModel for the class, everything should be fine, no matter what we call the protocol. 🍑

If we want to break everybody's build when they upgrade to 2.0 so we can make sure they follow our upgrade guide, then this may be a good opportunity to rename MTLModel the class.

@jspahrsummers
Copy link
Member

@jspahrsummers jspahrsummers commented Feb 4, 2014

We'll probably have enough breakage no matter what. :trollface:

Seriously, though, I'm most interested in Doing Things Right™—I think that's the MTLModel name for the class and protocol. MTLObject reminds me of scary general-purpose frameworks, like Omni's Foundation.

robb added 2 commits Feb 26, 2014
Conflicts:
	Mantle/MTLManagedObjectAdapter.m
	MantleTests/MTLTestModel.h
@robb
Copy link
Member Author

@robb robb commented Feb 27, 2014

Improved some of the documentation

@@ -187,13 +185,13 @@ - (id)initWithJSONDictionary:(NSDictionary *)JSONDictionary modelClass:(Class)mo
}
}

_model = [self.modelClass modelWithDictionary:dictionaryValue error:error];
_model = [[self.modelClass alloc ] initWithDictionary:dictionaryValue error:error];

This comment has been minimized.

@jspahrsummers

jspahrsummers Mar 13, 2014
Member

Accidental extra space here.

@@ -157,9 +157,9 @@ - (id)modelFromManagedObject:(NSManagedObject *)managedObject processedObjects:(
// a new object to be stored in this variable (and we don't want ARC to
// double-free or leak the old or new values).
__autoreleasing id replaceableValue = value;
if (![model validateValue:&replaceableValue forKey:key error:error]) return NO;
if (![(NSObject *)model validateValue:&replaceableValue forKey:key error:error]) return NO;

This comment has been minimized.

@jspahrsummers

jspahrsummers Mar 13, 2014
Member

Can we just type the variable as NSObject * in this scope?

@@ -593,7 +615,7 @@ - (NSPredicate *)uniquingPredicateForModel:(MTLModel<MTLManagedObjectSerializing

NSAssert(managedObjectKey != nil, @"%@ must map to a managed object key.", propertyKey);

id value = [model valueForKeyPath:propertyKey];
id value = [(NSObject *)model valueForKeyPath:propertyKey];

This comment has been minimized.

@jspahrsummers

jspahrsummers Mar 13, 2014
Member

As above.

@@ -10,7 +10,7 @@
#import "EXTRuntimeExtensions.h"
#import "EXTScope.h"
#import "MTLReflection.h"
#import <objc/runtime.h>
#import "MTLModel.h"

This comment has been minimized.

@jspahrsummers

jspahrsummers Mar 13, 2014
Member

This should be taken care of in the header.

@@ -7,22 +7,17 @@
//

#import <Foundation/Foundation.h>
#import "MTLModel.h"

This comment has been minimized.

This comment has been minimized.

@robb

robb Mar 13, 2014
Author Member

I'ma blame AppCode

// The default implementations of <NSCopying>, -hash, and -isEqual: make use of
// the +propertyKeys method.
@interface MTLModel : NSObject <NSCopying>
@protocol MTLModel <NSObject, NSCopying>

This comment has been minimized.

@jspahrsummers

jspahrsummers Mar 13, 2014
Member

Can you document the purpose of the protocol (and, specifically, the split between it and the class)?

//
// This is the designated initializer for this class.
- (instancetype)init;
// Cmbines the values corresponding to all +propertyKeys into a dictionary,

This comment has been minimized.

@jspahrsummers

jspahrsummers Mar 13, 2014
Member

*Combines

Extraneous space, too.

// -mergeValueForKey:fromModel: for each key in +propertyKeys.
//
// `model` must be an instance of the receiver's class or a subclass thereof.
- (void)mergeValuesForKeysFromModel:(id<MTLModel>)model;

This comment has been minimized.

@jspahrsummers

jspahrsummers Mar 13, 2014
Member

As a general rule of thumb, anything that callers can accomplish on their own—using only protocol methods—I'd prefer to leave out of the protocol, since it's just boilerplate to implement.

This comment has been minimized.

@robb

robb Mar 13, 2014
Author Member

I'd like to have a well-defined update hook in case somebody wants to write an updating adapter.
What if we made it @optional?

This comment has been minimized.

@jspahrsummers

jspahrsummers Mar 14, 2014
Member

Isn't that -mergeValueForKey:fromModel:? I'm only proposing removing the bulk variant, since it's just the aforementioned plus +propertyKeys.

This comment has been minimized.

@robb

robb Mar 14, 2014
Author Member

Yeah, you're right.

// key from the given model object, giving precedence to the other model object.
// Returns a new instance of the receiver initialized using
// -initWithDictionary:error:.
+ (instancetype)modelWithDictionary:(NSDictionary *)dictionaryValue error:(NSError **)error;

This comment has been minimized.

@jspahrsummers

jspahrsummers Mar 13, 2014
Member

I'd like to remove this entirely, but since this method could return a subclass for a class cluster, it seems useful to keep around. I don't like how it's separated from the protocol, though. :\

What do you think?

This comment has been minimized.

@robb

robb Mar 13, 2014
Author Member

Hmm, class clusters could bend -initWithDictionary: to their will but that seems needlessly complicated, moving this into the protocol would mean more busywork (and the method starting with model may be confusing if the protocol gets implemented by a SKSpriteNode subclass, for example).

Any reason you'd like to get rid of it, other than reduced surface area?

This comment has been minimized.

@jspahrsummers

jspahrsummers Mar 14, 2014
Member

Tell you what, why don't we require the class method in the protocol, and put the instance method onto MTLModel?

The instance method is only really exposed for subclassing anyways. The class method is what should be called (for class clustering reasons).

This comment has been minimized.

@robb

robb Mar 14, 2014
Author Member

👍

NSParameterAssert(key != nil);

SEL selector = MTLSelectorWithCapitalizedKeyPattern("merge", key, "FromModel:");
if (![self respondsToSelector:selector]) {
if (model != nil) {
[self setValue:[model valueForKey:key] forKey:key];
[self setValue:[(NSObject *)model valueForKey:key] forKey:key];

This comment has been minimized.

@jspahrsummers

jspahrsummers Mar 13, 2014
Member

As above.

@jspahrsummers
Copy link
Member

@jspahrsummers jspahrsummers commented Mar 13, 2014

🎎

@robb
Copy link
Member Author

@robb robb commented Mar 13, 2014

Let me know what you think of this protocol introduction.

@jspahrsummers
Copy link
Member

@jspahrsummers jspahrsummers commented Mar 14, 2014

Documentation looks . Just the open comment threads now.

@robb
Copy link
Member Author

@robb robb commented Mar 14, 2014

📟

@robb
Copy link
Member Author

@robb robb commented Mar 14, 2014

merging

Conflicts:
	Mantle/MTLModel.h
	MantleTests/MTLTestModel.h
	MantleTests/MTLTestModel.m
@robb
Copy link
Member Author

@robb robb commented Mar 14, 2014

merged

// Conforms to MTLJSONSerializing but does not inherit from the MTLModel class.
@interface MTLConformingModel : NSObject <MTLJSONSerializing>

- (instancetype)initWithDictionary:(NSDictionary *)dictionaryValue error:(NSError **)error;

This comment has been minimized.

@jspahrsummers

jspahrsummers Mar 14, 2014
Member

This doesn't need to be declared publicly unless it's being used in tests.

@jspahrsummers
Copy link
Member

@jspahrsummers jspahrsummers commented Mar 14, 2014

☎️

@robb
Copy link
Member Author

@robb robb commented Mar 14, 2014

📱

@jspahrsummers

This comment has been minimized.

Copy link
Member

@jspahrsummers jspahrsummers commented on af1ea58 Mar 14, 2014

Don't technically need this either, but it's not a big deal. :P

@jspahrsummers
Copy link
Member

@jspahrsummers jspahrsummers commented Mar 14, 2014

📲

jspahrsummers added a commit that referenced this pull request Mar 14, 2014
@jspahrsummers jspahrsummers merged commit c7ffb81 into 2.0-development Mar 14, 2014
1 check passed
1 check passed
@hubot
default Build #1131083 succeeded in 41s
Details
@jspahrsummers jspahrsummers deleted the model-protocol branch Mar 14, 2014
@priteshshah1983
Copy link
Contributor

@priteshshah1983 priteshshah1983 commented Mar 14, 2014

Is this going to be released soon?

@jspahrsummers
Copy link
Member

@jspahrsummers jspahrsummers commented Mar 14, 2014

@priteshshah1983 This is currently part of the in-development 2.0 version of Mantle (occurring on the 2.0-development branch). We haven't yet created plans around releasing it, since it's still actively being worked on and changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants