ObjectiveCDictionaryExtension to correctly handle maps of root objects #81

nguyenhuy · 2017-09-21T20:44:27Z

No description provided.

nguyenhuy · 2017-09-21T20:45:43Z

plank.xcodeproj/project.pbxproj

 			sourceTree = "<group>";
+			tabWidth = 4;


Just a little ride along :)

We may should have a different PR for it, but I would let @rahul-malik make the call

maicki · 2017-09-21T21:04:41Z

Examples/Cocoa/Sources/objc/Pin.m

@@ -307,8 +346,10 @@ - (BOOL)isEqualToPin:(Pin *)anObject
        (_counts == anObject.counts || [_counts isEqualToDictionary:anObject.counts]) &&
        (_descriptionText == anObject.descriptionText || [_descriptionText isEqualToString:anObject.descriptionText]) &&
        (_creator == anObject.creator || [_creator isEqualToDictionary:anObject.creator]) &&
+        (_tags == anObject.tags || [_tags isEqual:anObject.tags]) &&


Can we use isEqualToArray: here?

Good point. Here is the PR :)

maicki · 2017-09-21T21:08:22Z

plank.xcodeproj/project.pbxproj

 			sourceTree = "<group>";
+			tabWidth = 4;


We may should have a different PR for it, but I would let @rahul-malik make the call

maicki · 2017-09-21T21:09:49Z

Examples/Cocoa/Sources/objc/Pin.m

@@ -676,6 +757,11 @@ - (void)setCreator:(NSDictionary<NSString *, User *> *)creator
    _creator = creator;
    _pinDirtyProperties.PinDirtyPropertyCreator = 1;
 }
+- (void)setTags:(NSArray<NSDictionary *> *)tags
+{
+    _tags = tags;


Should we copy in here and same for NSDictionary cases?

Yeap, but not in this PR :)

maicki · 2017-09-21T21:13:02Z

Examples/Cocoa/Sources/objc/Pin.m

+                    for (id obj0 in items) {
+                        if (obj0 != (id)kCFNull) {
+                            id tmp0 = nil;
+                            tmp0 = obj0;


Can obj0 ever nil in here as we iterate through an NSArray

Not that I know of.

So should we remove this tmp0 copy than and just add the obj0 in the result0 directly?

Agree, although these lines were generated by ObjectiveCInitExtension, so out of scope :)

maicki

LGTM, but would like to have @rahul-malik another look over it.

maicki · 2017-09-21T21:34:29Z

Examples/Cocoa/Sources/objc/Pin.m

+                    for (id obj0 in items) {
+                        if (obj0 != (id)kCFNull) {
+                            id tmp0 = nil;
+                            tmp0 = obj0;


So should we remove this tmp0 copy than and just add the obj0 in the result0 directly?

maicki · 2017-09-21T21:36:20Z

plank.xcodeproj/project.pbxproj

@@ -172,7 +172,9 @@
 				OBJ_23 /* Utility */,
 				OBJ_24 /* Products */,
 			);
+			indentWidth = 2;


Are 2 indentations the default we have?

Haha, I should have removed those lines. Sorry.

rahul-malik

Looks good, please address comments and let me know if you're ready to land

rahul-malik · 2017-09-21T22:27:08Z

Sources/Core/ObjectiveCDictionaryExtension.swift

-            switch type.unsafelyUnwrapped {
+        case .map(valueType: .none):
+            return [
+                ObjCIR.ifStmt("\(propIVarName) != nil") {[


You should use ObjCIR.ifElseStmt here :)

rahul-malik · 2017-09-21T22:27:29Z

Sources/Core/ObjCModelRenderer.swift

@@ -205,7 +205,7 @@ public struct ObjCModelRenderer: ObjCFileRenderer {
                    (.privateM, self.renderHash()),
                    (.publicM, self.renderMergeWithModel()),
                    (.publicM, self.renderMergeWithModelWithInitType()),
-                    (.publicM, self.renderGenerateDictionary())
+                    (self.isBaseClass ? .publicM : .privateM, self.renderGenerateDictionary())



rahul-malik · 2017-09-21T22:27:36Z

Examples/PDK/pin.json

+        },
+        "visual_search_attrs": {
+            "type": "object"
+        }


thanks for adding these cases!

- Update Pin example - Minor change in plank.xcodeproj

nguyenhuy · 2017-09-21T23:06:41Z

@rahul-malik @maicki Thanks for reviewing. Comments addressed!

rahul-malik · 2017-09-21T23:38:18Z

Sources/Core/ObjectiveCDictionaryExtension.swift

-                    return [
-                        ObjCIR.ifStmt("\(propIVarName) != nil && [NSValueTransformer allowsReverseTransformation]") {[
+                    return
+                        ObjCIR.ifElseStmt("\(propIVarName) != nil && [NSValueTransformer allowsReverseTransformation]") {[


I don't think I saw this the first time, don't we have to get the actual NSValueTransformer subclass here?

We should be able to get the transformer instance with dateValueTransformerKey, get it's class and then ask it if it allowsReverseTransformation

rahul-malik · 2017-09-21T23:40:07Z

Thanks @nguyenhuy - I caught another issue in that code if you don't mind fixing. I'll look again in the morning

ghost · 2017-09-22T10:41:53Z

	1 Warning
⚠️	Any source code changes should have an entry in CHANGELOG.md or have #trivial in their title.

Generated by 🚫 Danger

nguyenhuy · 2017-09-22T10:43:50Z

@rahul-malik Done!

nguyenhuy commented Sep 21, 2017

View reviewed changes

maicki reviewed Sep 21, 2017

View reviewed changes

nguyenhuy force-pushed the HNDictExtension branch from 830d86f to 29794bd Compare September 21, 2017 21:45

nguyenhuy mentioned this pull request Sep 21, 2017

Compare NSArrays using -isEqualToArray: #83

Merged

rahul-malik approved these changes Sep 21, 2017

View reviewed changes

nguyenhuy added 5 commits September 21, 2017 23:40

ObjectiveCDictionaryExtension to correctly handle maps of root objects

867cf34

- Update Pin example - Minor change in plank.xcodeproj

Handle inheritance

6c24d81

Remove unnecessary changes

8e23bed

Update JS example as well

4a0d3c9

Regenerate pin.json

d293d0e

nguyenhuy force-pushed the HNDictExtension branch from 0776381 to d293d0e Compare September 21, 2017 22:41

nguyenhuy added 4 commits September 21, 2017 23:44

Revert changes in xcodeproj

0655252

Use ifElseStmt

3d31099

Use tabs in pin.json

fd66869

Regenerate ObjC examples

aeeae40

rahul-malik suggested changes Sep 21, 2017

View reviewed changes

Address Rahul's comments

1548a1e

rahul-malik approved these changes Sep 22, 2017

View reviewed changes

rahul-malik merged commit 6399daf into pinterest:master Sep 22, 2017

nguyenhuy deleted the HNDictExtension branch September 22, 2017 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ObjectiveCDictionaryExtension to correctly handle maps of root objects #81

ObjectiveCDictionaryExtension to correctly handle maps of root objects #81

nguyenhuy commented Sep 21, 2017 •

edited

Loading

nguyenhuy Sep 21, 2017

maicki Sep 21, 2017

maicki Sep 21, 2017

nguyenhuy Sep 21, 2017 •

edited

Loading

maicki Sep 21, 2017

maicki Sep 21, 2017

nguyenhuy Sep 21, 2017 •

edited

Loading

maicki Sep 21, 2017

nguyenhuy Sep 21, 2017

maicki Sep 21, 2017

nguyenhuy Sep 21, 2017

maicki left a comment

maicki Sep 21, 2017

maicki Sep 21, 2017

nguyenhuy Sep 21, 2017

rahul-malik left a comment

rahul-malik Sep 21, 2017

rahul-malik Sep 21, 2017

rahul-malik Sep 21, 2017

nguyenhuy commented Sep 21, 2017 •

edited

Loading

rahul-malik Sep 21, 2017

rahul-malik Sep 21, 2017

rahul-malik commented Sep 21, 2017

ghost commented Sep 22, 2017

nguyenhuy commented Sep 22, 2017

ObjectiveCDictionaryExtension to correctly handle maps of root objects #81

ObjectiveCDictionaryExtension to correctly handle maps of root objects #81

Conversation

nguyenhuy commented Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy Sep 21, 2017 • edited Loading

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

maicki 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

rahul-malik 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

nguyenhuy commented Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahul-malik commented Sep 21, 2017

ghost commented Sep 22, 2017

nguyenhuy commented Sep 22, 2017

nguyenhuy commented Sep 21, 2017 •

edited

Loading

nguyenhuy Sep 21, 2017 •

edited

Loading

nguyenhuy Sep 21, 2017 •

edited

Loading

nguyenhuy commented Sep 21, 2017 •

edited

Loading