-
Notifications
You must be signed in to change notification settings - Fork 488
[ObjC] generating: `- (NSDictionary *) toDict;' for Objective C records #158
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,14 @@ + (nonnull instancetype)itemListWithItems:(nonnull NSArray<NSString *> *)items | |
|
||
- (NSString *)description | ||
{ | ||
return [NSString stringWithFormat:@"<%@ %p items:%@>", self.class, self, self.items]; | ||
return [NSString stringWithFormat:@"<class, %@ : %p, dict, %@>", self.class, self, [[self toDict] description]]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused about the use of comma and colon here as separators, and find this format hard to understand. I suggest going back to the old format, or maybe using a name=value or name:value layout more consistently across the 3 values you're providing. |
||
} | ||
|
||
- (NSDictionary *)toDict | ||
{ | ||
#define _djinni_hide_null_(_o_) ((_o_)?(_o_):([NSNull null])) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was discussed elsewhere, though I can't see the discussion when scrolling around now. I think this use of a macro is really awkward. I support the alternative of having the generator simply generate the required code for each field, using ?: directly for optional fields, and not for non-optional fields. In this case, the "items" field isn't optional, so there shouldn't be any need to check for null anyway. |
||
|
||
return @{@"__class_name__": [self.class description], @"items": _djinni_hide_null_(self.items)}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the use of class_name here a standard thing in ObjC for any other specific format, or something made up for Djinni? It's redundant with the class name output in description currently. |
||
} | ||
|
||
@end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -205,6 +205,8 @@ class ObjcGenerator(spec: Spec) extends Generator(spec) { | |
|
||
writeInitializer("-", "init") | ||
if (!r.ext.objc) writeInitializer("+", IdentStyle.camelLower(objcName)) | ||
w.wl(s""); | ||
w.wl(s"- (nonnull NSDictionary *) toDict;"); | ||
|
||
for (f <- r.fields) { | ||
w.wl | ||
|
@@ -389,13 +391,28 @@ class ObjcGenerator(spec: Spec) extends Generator(spec) { | |
w.wl("- (NSString *)description") | ||
w.braced { | ||
w.w(s"return ").nestedN(2) { | ||
w.w("[NSString stringWithFormat:@\"<%@ %p") | ||
w.w("[NSString stringWithFormat:@\"<%@ %p: dict, %@") | ||
w.w(">\", self.class, self, [[self toDict] description]") | ||
w.wl("];") | ||
} | ||
} | ||
w.wl | ||
|
||
for (f <- r.fields) w.w(s" ${idObjc.field(f.ident)}:%@") | ||
w.w(">\", self.class, self") | ||
w.wl("- (NSDictionary *)toDict") | ||
w.braced { | ||
w.wl("#define _djinni_hide_null_(_o_) ((_o_)?(_o_):([NSNull null]))") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is a macro really necessary? It's a code generator, no reason to avoid repetetive output. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, adding a nil value into a NSDictionary will throw an exception and crash. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not questioning the operation itself, but whether it has to implemented with a macro. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i would love to add it to a common file, but unfortunately the generated ObjC code, doesn't define a file that other files include (yet) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mknejp i see. i though that doing this with a macro will be more readable, (instead of generating a bunch of repeated code) also, if i convert the macro into a function, it will be more efficient as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so can convert to a function or just unroll the code to be repeated, whatever you think is right There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest to just unroll it. The generated code only needs to be looked at by people who are developing Djinni, it is not targetted at users. Furthermore, only fields that are marked as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the record, I support unrolling, as well as omitting the code where it's unnecessary (non-optional fields). |
||
w.wl("") | ||
w.w(s"return ").nestedN(2) { | ||
w.w("@{") | ||
w.w("@\"__class_name__\": [self.class description]") | ||
|
||
for (f <- r.fields) { | ||
w.w(", ") | ||
w.w(", @\"") | ||
w.w(idObjc.field(f.ident)) | ||
w.w("\": ") | ||
|
||
w.w("_djinni_hide_null_(") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is nn relevant here? The nn feature is only used for interfaces, and this feature seems to be for records, which aren't (currently) able to contain interfaces. All datatypes in ObjC records will be non-null unless they were declared optional in the .djinni file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i have replied to the first question above, please take a look. |
||
|
||
f.ty.resolved.base match { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this really does need to apply toDict recursively for nested records in order to be a complete feature. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please see the comment / discussion about this above. |
||
case MOptional => w.w(s"self.${idObjc.field(f.ident)}") | ||
case t: MPrimitive => w.w(s"@(self.${idObjc.field(f.ident)})") | ||
|
@@ -411,9 +428,11 @@ class ObjcGenerator(spec: Spec) extends Generator(spec) { | |
} | ||
case _ => w.w(s"self.${idObjc.field(f.ident)}") | ||
} | ||
|
||
w.w(")") | ||
} | ||
} | ||
w.wl("];") | ||
w.wl("};") | ||
} | ||
w.wl | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,4 +12,6 @@ | |
- (nonnull instancetype)init; | ||
+ (nonnull instancetype)emptyRecord; | ||
|
||
- (nonnull NSDictionary *) toDict; | ||
|
||
@end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel like
toDict
is in line with Cocoa naming conventionsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please suggest an alternative name that in your opinion follows the convention.
(in favor of saving time and effort, next time, please make a suggestion next to the original comment :) )