Custom setter with error out parameter #564

Krizai · 2017-02-09T21:34:07Z

Implemented custom possibility to use custom setter with error out parameter

It allows to use another JSONModel initializers inside custom setter and propagate error to the topmost. Like here:


- (void)setIconsWithNSDictionary:(NSDictionary *)dictionary error:(NSError**) error
{
    NSMutableDictionary* icons = [NSMutableDictionary new];
    __block NSError* blockError = nil;
    [dictionary enumerateKeysAndObjectsUsingBlock:^(id  _Nonnull key, id  _Nonnull obj, BOOL * _Nonnull stop) {
        NSError* localError = nil;
        PFEffectImageJS* effectImage = [[PFEffectImageJS alloc] initWithDictionary:obj error:&localError];
        if(localError){
            blockError = localError;
            *stop = YES;
            return;
        }
        icons[key] = effectImage;
    }];
    
    if(error){
        *error = blockError;
    }

    self.icons = icons;
}

billinghamj · 2017-02-10T16:52:12Z

Ah excellent! :) I have been wanting this for a while - much cleaner than the previous solution. Will review and probably make a couple of tweaks.

billinghamj

Okay, have reviewed in detail. Most of the changes are stylistic to fit in with the newer code in the project (I know much of the older code is quite different).

Finally, please add tests for both the old and new use of the setters (with BOOL and void return types), add something to show that the old method is deprecated, and update the readme please

billinghamj · 2017-02-21T11:19:45Z

JSONModel/JSONModel/JSONModel.m

-            if ([self __customSetValue:jsonValue forProperty:property]) {
+            NSError* customSetErr = nil;
+            if ([self __customSetValue:jsonValue forProperty:property error:&customSetErr]) {
+                if((err != nil) && (customSetErr != nil))


Please update to:

if (err != nil && customSetErr != nil) *err = customSetErr;

billinghamj · 2017-02-21T11:19:56Z

JSONModel/JSONModel/JSONModel.m

@@ -327,7 +328,12 @@ -(BOOL)__importDictionary:(NSDictionary*)dict withKeyMapper:(JSONKeyMapper*)keyM

            // check for custom setter, than the model doesn't need to do any guessing
            // how to read the property's value from JSON
-            if ([self __customSetValue:jsonValue forProperty:property]) {
+            NSError* customSetErr = nil;


Please change to NSError *customSetErr

Do you use any formatter tool to take care of formatting. Something like uncrustify?

Unfortunately not at present, no

billinghamj · 2017-02-21T11:21:40Z

JSONModel/JSONModel/JSONModel.m

+-(void)__addCustomSetterForName:(NSString*) name
+                     withSuffix:(NSString*) suffix
+                            key:(NSString*) key
+                     toProperty:(JSONModelClassProperty*) p{


Please change to something more like:

- (void)__addCustomSetterToProperty:(JSONModelClassProperty *)property withValueType:(NSString *)valueType {

all args/params on a single line

curly brace on a new line

space before pointer asterisks, rather than after

no space before variable names

more clear variable/param naming

avoiding unneeded params

billinghamj · 2017-02-21T11:24:27Z

JSONModel/JSONModel/JSONModel.m

+        p.customSetters[key] = [[JSONModelCustomSetter alloc] initWithValue:setterValue withErrorOutParam:NO];
+    }
+
+    SEL setterWithError = NSSelectorFromString([NSString stringWithFormat:@"set%@With%@:error:", name, suffix]);


Check for the error setter first, then return within the respondsToSelector conditional block

billinghamj · 2017-02-21T11:24:43Z

JSONModel/JSONModel/JSONModel.m

@@ -841,22 +861,30 @@ -(id)__reverseTransform:(id)value forProperty:(JSONModelClassProperty*)property
 }

 #pragma mark - custom transformations
- (BOOL)__customSetValue:(id <NSObject>)value forProperty:(JSONModelClassProperty *)property
+- (BOOL)__customSetValue:(id <NSObject>)value forProperty:(JSONModelClassProperty *)property error:(NSError**) error


Space before pointer asterisks

billinghamj · 2017-02-21T11:33:44Z

JSONModel/JSONModel/JSONModel.m

+
+    if ([self respondsToSelector:setterWithError]){
+        NSValue* setterValue = [NSValue valueWithBytes:&setterWithError objCType:@encode(SEL)];
+        p.customSetters[key] = [[JSONModelCustomSetter alloc] initWithValue:setterValue withErrorOutParam:YES];


Init with SEL object directly, not the NSValue

(NSValue was only used because an NSDictionary cannot hold a value type - e.g. SEL, thus there is no need to use NSValue at all)

billinghamj · 2017-02-21T11:35:42Z

JSONModel/JSONModel/JSONModel.m

        return NO;

+    SEL setter = nil;
+    [customSetter.value getValue:&setter];


Pull SEL directly from JSONModelCustomSetter property, rather than using an NSValue

billinghamj · 2017-02-21T11:36:02Z

JSONModel/JSONModel/JSONModelCustomSetter.h

+
+@interface JSONModelCustomSetter : NSObject
+
+@property ( nonatomic, readonly, strong) NSValue* value;


Change to SEL type

billinghamj · 2017-02-21T11:37:22Z

JSONModel/JSONModel/JSONModelCustomSetter.h

+@property ( nonatomic, readonly, strong) NSValue* value;
+@property ( nonatomic, readonly, assign) BOOL withErrorOutParam;
+
+- (instancetype)initWithValue:(NSValue*)value withErrorOutParam:(BOOL) withErrorOutParam;


Change to:

- (instancetype)initWithSelector:(SEL)selector;

Determine the presence of the error param based on the selector - don't pass this in

billinghamj · 2017-02-21T11:42:24Z

JSONModel/JSONModel/JSONModel.m

+    if (p.customSetters[key])
+        return;
+
+    SEL setter = NSSelectorFromString([NSString stringWithFormat:@"set%@With%@:", name, suffix]);


Please move all this logic into JSONModelCustomSetter - probably something like:

JSONModelCustomSetter *setter = [JSONModelCustomSetter setterForObj:self propertyName:name valueType:suffix]; if (setter) p.customSetters[key] = setter;

From my point of view, anyway, I should first check, does the selector exist, and only then create JSONModelCustomSetter. So I can only hide into the class the magic of wrapping into the NSValue ( which anyway will be removed)

If the selector doesn't exist, setterForObj would return nil.

billinghamj · 2017-02-21T17:44:53Z

Also needs to be rebased

…rameter

Krizai · 2017-02-25T17:16:43Z

Do you think old method should be deprecated? I would leave both of them, as in many cases there is no need for error processing in custom setter.

Krizai · 2017-02-25T17:22:27Z

Please check new update

billinghamj · 2017-03-24T12:30:31Z

Yes the old method should be deprecated - we avoid supporting more than one way of doing a given thing at once generally, as it keeps it more maintainable.

Krizai · 2017-05-06T16:53:15Z

Added the warning about deprecation of the old method

ghost mentioned this pull request Feb 14, 2017

override parsed value #565

Closed

billinghamj requested changes Feb 21, 2017

View reviewed changes

Implemented custom possibility to use custom setter with error out pa…

13ea1b6

…rameter

Krizai force-pushed the custom_setter_with_error branch from 524b0cb to 13ea1b6 Compare February 23, 2017 20:55

Dmytro Povolotskyi added 2 commits February 25, 2017 18:21

Code style fixes. Refactoring of the CustomSetter.

3dcf5db

Added tests for custom setter

8daccf7

Deprecation warning added

4b11232

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom setter with error out parameter #564

Custom setter with error out parameter #564

Krizai commented Feb 9, 2017

billinghamj commented Feb 10, 2017

billinghamj left a comment

billinghamj Feb 21, 2017

billinghamj Feb 21, 2017

Krizai Feb 24, 2017

billinghamj Feb 24, 2017

billinghamj Feb 21, 2017

billinghamj Feb 21, 2017

billinghamj Feb 21, 2017

billinghamj Feb 21, 2017

billinghamj Feb 21, 2017

billinghamj Feb 21, 2017

billinghamj Feb 21, 2017

billinghamj Feb 21, 2017

billinghamj Feb 21, 2017

Krizai Feb 24, 2017 •

edited

billinghamj Feb 24, 2017

billinghamj commented Feb 21, 2017

Krizai commented Feb 25, 2017

Krizai commented Feb 25, 2017

billinghamj commented Mar 24, 2017

Krizai commented May 6, 2017


		@interface JSONModelCustomSetter : NSObject

		@property ( nonatomic, readonly, strong) NSValue* value;

Custom setter with error out parameter #564

Are you sure you want to change the base?

Custom setter with error out parameter #564

Conversation

Krizai commented Feb 9, 2017

billinghamj commented Feb 10, 2017

billinghamj 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

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

Krizai Feb 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billinghamj commented Feb 21, 2017

Krizai commented Feb 25, 2017

Krizai commented Feb 25, 2017

billinghamj commented Mar 24, 2017

Krizai commented May 6, 2017

Krizai Feb 24, 2017 •

edited