Added array mapping transformer and refactored JSON array transformer #248

Merged
merged 10 commits into from Mar 9, 2014

Conversation

Projects
None yet
2 participants
@dcaunt
Member

dcaunt commented Feb 22, 2014

See #191

If this looks good I can add tests. All of the tests for mtl_JSONArrayTransformerWithModelClass: continue to work without any changes.

Some detail in the error key NSLocalizedDescriptionKey returned on mtl_JSONArrayTransformerWithModelClass:'s failure is now lost but we could check for an error in the array mapping transformer and modify the error message.

Unfortunately the diff looks pretty crazy for adding a new method.

@robb

This comment has been minimized.

Show comment Hide comment
@robb

robb Feb 22, 2014

Owner

Maybe its simpler doing a proper NSValueTransformer subclass rather than having two huge blocks?

Owner

robb commented Feb 22, 2014

Maybe its simpler doing a proper NSValueTransformer subclass rather than having two huge blocks?

@dcaunt

This comment has been minimized.

Show comment Hide comment
@dcaunt

dcaunt Feb 24, 2014

Member

@robb How do you want me to implement that, can you be more specific?

Member

dcaunt commented Feb 24, 2014

@robb How do you want me to implement that, can you be more specific?

@robb

This comment has been minimized.

Show comment Hide comment
@robb

robb Feb 24, 2014

Owner

I was thinking it could make sense to subclass NSValueTransformer and implement -transformedValue:success:error: there rather than using Mantle's block based transformer. That could end up more readable.

We'd maybe still want a +mtl_arrayMappingTransformerWithTransformer: convenience method, so I'm not sure if it's worth it

Owner

robb commented Feb 24, 2014

I was thinking it could make sense to subclass NSValueTransformer and implement -transformedValue:success:error: there rather than using Mantle's block based transformer. That could end up more readable.

We'd maybe still want a +mtl_arrayMappingTransformerWithTransformer: convenience method, so I'm not sure if it's worth it

@dcaunt

This comment has been minimized.

Show comment Hide comment
@dcaunt

dcaunt Feb 25, 2014

Member

I can refactor if there's a better way, though not using Mantle's block based transformer makes it more difficult to support both reversible and non-reversible transformers.

Member

dcaunt commented Feb 25, 2014

I can refactor if there's a better way, though not using Mantle's block based transformer makes it more difficult to support both reversible and non-reversible transformers.

@robb

This comment has been minimized.

Show comment Hide comment
@robb

robb Feb 25, 2014

Owner

good point 👍

Owner

robb commented Feb 25, 2014

good point 👍

+ if (![values isKindOfClass:NSArray.class]) {
+ if (error != NULL) {
+ NSDictionary *userInfo = @{
+ NSLocalizedDescriptionKey: NSLocalizedString(@"Could not transform non-array type", @""),

This comment has been minimized.

Show comment Hide comment
@robb

robb Feb 25, 2014

Owner

the whitespace looks kinda off here

@robb

robb Feb 25, 2014

Owner

the whitespace looks kinda off here

+ return transformedValues;
+ };
+ }
+ if (reverseBlock) {

This comment has been minimized.

Show comment Hide comment
@robb

robb Feb 25, 2014

Owner

this should be (reverseBlock != nil)

@robb

robb Feb 25, 2014

Owner

this should be (reverseBlock != nil)

@robb

This comment has been minimized.

Show comment Hide comment
@robb

robb Feb 25, 2014

Owner

I think this is on the right track, feel free to add tests

Owner

robb commented Feb 25, 2014

I think this is on the right track, feel free to add tests

@dcaunt

This comment has been minimized.

Show comment Hide comment
@dcaunt

dcaunt Feb 25, 2014

Member

Cool, I'll sort out the tests and tidy up the above

On Tuesday, 25 February 2014, Robert Böhnke notifications@github.com
wrote:

I think this is on the right track, feel free to add tests


Reply to this email directly or view it on GitHubhttps://github.com/MantleFramework/Mantle/pull/248#issuecomment-36052487
.

Member

dcaunt commented Feb 25, 2014

Cool, I'll sort out the tests and tidy up the above

On Tuesday, 25 February 2014, Robert Böhnke notifications@github.com
wrote:

I think this is on the right track, feel free to add tests


Reply to this email directly or view it on GitHubhttps://github.com/MantleFramework/Mantle/pull/248#issuecomment-36052487
.

@@ -46,6 +46,17 @@
// array elements back and forth.
+ (NSValueTransformer<MTLTransformerErrorHandling> *)mtl_JSONArrayTransformerWithModelClass:(Class)modelClass;
+// An optionally reversible transformer which applies the given transformer to
+// each element of an array

This comment has been minimized.

Show comment Hide comment
@robb

robb Feb 28, 2014

Owner

I think there is a full stop missing at the end of the line here.

@robb

robb Feb 28, 2014

Owner

I think there is a full stop missing at the end of the line here.

+// reversible.
+//
+// Returns a transformer which applies a transformation to each element of an
+// array

This comment has been minimized.

Show comment Hide comment
@robb

robb Feb 28, 2014

Owner

I think there is a full stop missing at the end of the line here.

@robb

robb Feb 28, 2014

Owner

I think there is a full stop missing at the end of the line here.

};
-
+

This comment has been minimized.

Show comment Hide comment
@robb

robb Feb 28, 2014

Owner

Trailing whitespace

@robb

robb Feb 28, 2014

Owner

Trailing whitespace

};
-
+

This comment has been minimized.

Show comment Hide comment
@robb

robb Feb 28, 2014

Owner

Trailing whitespace

@robb

robb Feb 28, 2014

Owner

Trailing whitespace

}
+ (NSValueTransformer<MTLTransformerErrorHandling> *)mtl_valueMappingTransformerWithDictionary:(NSDictionary *)dictionary {
NSParameterAssert(dictionary != nil);
NSParameterAssert(dictionary.count == [[NSSet setWithArray:dictionary.allValues] count]);
-
+

This comment has been minimized.

Show comment Hide comment
@robb

robb Feb 28, 2014

Owner

Trailing whitespace.

@robb

robb Feb 28, 2014

Owner

Trailing whitespace.

+ if (result == nil) {
+ if (error != NULL) {
+ NSDictionary *userInfo = @{
+ NSLocalizedDescriptionKey: NSLocalizedString(@"Could not find associated value", @""),

This comment has been minimized.

Show comment Hide comment
@robb

robb Feb 28, 2014

Owner

Can you tweak the indentation here and in the reverse block?

@robb

robb Feb 28, 2014

Owner

Can you tweak the indentation here and in the reverse block?

@robb

This comment has been minimized.

Show comment Hide comment
@robb

robb Feb 28, 2014

Owner

Ah, sorry, hadn't realized this wasn't ready for review yet

Owner

robb commented Feb 28, 2014

Ah, sorry, hadn't realized this wasn't ready for review yet

@robb robb added the enhancement label Feb 28, 2014

@robb robb added this to the 2.0 milestone Feb 28, 2014

@robb

This comment has been minimized.

Show comment Hide comment
@robb

robb Mar 6, 2014

Owner

Looks good, is this ready to go?

Owner

robb commented Mar 6, 2014

Looks good, is this ready to go?

@dcaunt

This comment has been minimized.

Show comment Hide comment
@dcaunt

dcaunt Mar 6, 2014

Member

Still need to add some tests but the implementation is done.

Member

dcaunt commented Mar 6, 2014

Still need to add some tests but the implementation is done.

@dcaunt

This comment has been minimized.

Show comment Hide comment
@dcaunt

dcaunt Mar 8, 2014

Member

@robb I've added some tests but I'm a bit rusty on testing so let me know if you have any feedback

Member

dcaunt commented Mar 8, 2014

@robb I've added some tests but I'm a bit rusty on testing so let me know if you have any feedback

@@ -171,6 +171,60 @@
});
});
+describe(@"array mapping transformer", ^{
+ __block NSValueTransformer *transformer;
+ __block NSArray *URLStrings;

This comment has been minimized.

Show comment Hide comment
@robb

robb Mar 9, 2014

Owner

Since the URLs and strings are immutable, I think we could do something like:

NSArray *URLStrings = @[
    @"https://github.com",
    @"https://github.com/MantleFramework",
    @"https://apple.com]
];

NSArray *URLs = @[
    [NSURL URLwithString:@"https://github.com"],
    [NSURL URLwithString:@"https://github.com/MantleFramework"],
    [NSURL URLwithString:@"https://apple.com"]
];
@robb

robb Mar 9, 2014

Owner

Since the URLs and strings are immutable, I think we could do something like:

NSArray *URLStrings = @[
    @"https://github.com",
    @"https://github.com/MantleFramework",
    @"https://apple.com]
];

NSArray *URLs = @[
    [NSURL URLwithString:@"https://github.com"],
    [NSURL URLwithString:@"https://github.com/MantleFramework"],
    [NSURL URLwithString:@"https://apple.com"]
];

This comment has been minimized.

Show comment Hide comment
@dcaunt

dcaunt Mar 9, 2014

Member

Good point, I've made a new commit

@dcaunt

dcaunt Mar 9, 2014

Member

Good point, I've made a new commit

This comment has been minimized.

Show comment Hide comment
@robb

robb Mar 9, 2014

Owner

Can you also move them out of the beforeEach block and remove the __block qualifier?

@robb

robb Mar 9, 2014

Owner

Can you also move them out of the beforeEach block and remove the __block qualifier?

+ });
+
+ it(@"should apply the transformer to each element in reverse", ^{
+ expect([transformer.class allowsReverseTransformation]).to.beTruthy();

This comment has been minimized.

Show comment Hide comment
@robb

robb Mar 9, 2014

Owner

If the compiler let's you, this should be transformer.class.allowsReverseTransformation.

@robb

robb Mar 9, 2014

Owner

If the compiler let's you, this should be transformer.class.allowsReverseTransformation.

This comment has been minimized.

Show comment Hide comment
@dcaunt

dcaunt Mar 9, 2014

Member

Unfortunately it doesn't

@dcaunt

dcaunt Mar 9, 2014

Member

Unfortunately it doesn't

@robb

This comment has been minimized.

Show comment Hide comment
@robb

robb Mar 9, 2014

Owner

What are your thoughts on using nested describe blocks here for readability?

E.g.:

describe(@"+mtl_arrayMappingTransformerWithTransformer:", ^{
    describe(@"when called with a non-reversible transformer", ^{
        it(@"should not allow reverse transformation", ^{
            // check the allowsReverseTransformation flag
        });

        it(@"should apply the transformer", ^{
            // ...

            expect([transformer transformedValue:URLStrings]).to.equal(URLs);
        });
    });

    describe(@"when called with a reversible transformer", ^{
        it(@"should allow reverse transformation", ^{
            // check the allowsReverseTransformation flag
        });

        it(@"should apply the transformer in both directions", ^{
            // ...

            NSArray *transformed = [transformer transformedValue:URLStrings];

            expect(transformed).to.equal(URLs);

            expect([transformer reverseTransformedValue:transformed]).to.equal(URLStrings);
        });
    });
});
Owner

robb commented Mar 9, 2014

What are your thoughts on using nested describe blocks here for readability?

E.g.:

describe(@"+mtl_arrayMappingTransformerWithTransformer:", ^{
    describe(@"when called with a non-reversible transformer", ^{
        it(@"should not allow reverse transformation", ^{
            // check the allowsReverseTransformation flag
        });

        it(@"should apply the transformer", ^{
            // ...

            expect([transformer transformedValue:URLStrings]).to.equal(URLs);
        });
    });

    describe(@"when called with a reversible transformer", ^{
        it(@"should allow reverse transformation", ^{
            // check the allowsReverseTransformation flag
        });

        it(@"should apply the transformer in both directions", ^{
            // ...

            NSArray *transformed = [transformer transformedValue:URLStrings];

            expect(transformed).to.equal(URLs);

            expect([transformer reverseTransformedValue:transformed]).to.equal(URLStrings);
        });
    });
});

@robb robb self-assigned this Mar 9, 2014

@dcaunt

This comment has been minimized.

Show comment Hide comment
@dcaunt

dcaunt Mar 9, 2014

Member

I think it's a big improvement. The tests are a little more verbose than some of the other transformer tests, but I think that's no bad thing.

Member

dcaunt commented Mar 9, 2014

I think it's a big improvement. The tests are a little more verbose than some of the other transformer tests, but I think that's no bad thing.

@@ -171,6 +171,67 @@
});
});
+describe(@"array mapping transformer", ^{

This comment has been minimized.

Show comment Hide comment
@robb

robb Mar 9, 2014

Owner

Can you make this +mtl_arrayMappingTransformerWithTransformer:

@robb

robb Mar 9, 2014

Owner

Can you make this +mtl_arrayMappingTransformerWithTransformer:

+//
+// transformer - The transformer to apply to each element. If the transformer
+// is reversible, the transformer returned by this method will be
+// reversible.

This comment has been minimized.

Show comment Hide comment
@robb

robb Mar 9, 2014

Owner

… This argument must not be nil.

@robb

robb Mar 9, 2014

Owner

… This argument must not be nil.

@robb

This comment has been minimized.

Show comment Hide comment
@robb

robb Mar 9, 2014

Owner

💥

Owner

robb commented Mar 9, 2014

💥

robb added a commit that referenced this pull request Mar 9, 2014

Merge pull request #248 from dcaunt/array-mapping-transformer
[WIP] Added array mapping transformer and refactored JSON array transformer

@robb robb merged commit ecf3d76 into Mantle:2.0-development Mar 9, 2014

@dcaunt dcaunt deleted the dcaunt:array-mapping-transformer branch Mar 9, 2014

@dcaunt dcaunt unassigned robb Apr 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment