Custom setter with error out parameter by Krizai · Pull Request #564 · jsonmodel/jsonmodel

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

-            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

            // 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

+-(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

+        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


 #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

+
+    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

        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

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


Change to SEL type

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

+@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

+    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


		@interface JSONModelCustomSetter : NSObject

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

Conversation

Krizai commented Feb 9, 2017

Uh oh!

billinghamj commented Feb 10, 2017

Uh oh!

billinghamj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Krizai Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billinghamj commented Feb 21, 2017

Uh oh!

Krizai commented Feb 25, 2017

Uh oh!

Krizai commented Feb 25, 2017

Uh oh!

billinghamj commented Mar 24, 2017

Uh oh!

Krizai commented May 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Krizai Feb 24, 2017 •

edited

Loading