Skip to content
This repository was archived by the owner on Mar 26, 2020. It is now read-only.

[ObjC] generating: `- (NSDictionary *) toDict;' for Objective C records #158

Open
korovkin wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[ObjC] generating: `- (NSDictionary *) toDict;' for Objective C records #158

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
98747a0
[ObjC] generating: `- (NSDictionary *) toDict;' for Objective C records
korovkin Nov 5, 2015
78678ad
making `[description]` use the new `[toDict]` method
korovkin Nov 10, 2015
9b94082
updating generated test files
korovkin Nov 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions src/source/ObjcGenerator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -398,6 +400,7 @@ class ObjcGenerator(spec: Spec) extends Generator(spec) {
w.w(", ")
f.ty.resolved.base match {
case MOptional => w.w(s"self.${idObjc.field(f.ident)}")

case t: MPrimitive => w.w(s"@(self.${idObjc.field(f.ident)})")
case df: MDef => df.defType match {
case DEnum => w.w(s"@(self.${idObjc.field(f.ident)})")
Expand All @@ -417,6 +420,44 @@ class ObjcGenerator(spec: Spec) extends Generator(spec) {
}
w.wl

w.wl("- (NSDictionary *)toDict")
w.braced {
w.wl("#define _djinni_hide_null_(_o_) ((_o_)?(_o_):([NSNull null]))")

@mknejp mknejp Nov 5, 2015

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.
If it absolutely has to be a macro add an#undef when it's no longer needed.

@korovkin korovkin Nov 5, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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.

@mknejp mknejp Nov 5, 2015

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

@korovkin korovkin Nov 5, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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)

@korovkin korovkin Nov 6, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 self.<name> will be called only once, instead of twice (with the macro)

@korovkin korovkin Nov 6, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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

@mknejp mknejp Nov 6, 2015

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 optional can ever be nil, so the repetition is not as bad as it seems.

@artwyman artwyman Nov 12, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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(idObjc.field(f.ident))
w.w("\": ")

w.w("_djinni_hide_null_(")

@steipete steipete Nov 6, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the new nn nullability support we could check here and only ever call _djinni_hide_null_ when the object is optional.

@artwyman artwyman Nov 6, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

@korovkin korovkin Nov 11, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 {

@artwyman artwyman Nov 11, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.
The old code for "description" didn't need that structure in the generator, because it was done at run time. The string formatting using %@ would call description on the subobjects. That's not true for toDict, so I think in it's current form it's only useful as an implementation of description, or for single-level records. I think it should be made general before being merged.

@korovkin korovkin Nov 11, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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)})")
case df: MDef => df.defType match {
case DEnum => w.w(s"@(self.${idObjc.field(f.ident)})")
case _ => w.w(s"self.${idObjc.field(f.ident)}")

@artwyman artwyman Nov 6, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the conversion here shallow? I'd expect to see some recursion here, in the form of calls to [self.field toDict]. If the intent is to convert this record into a type which might be easily converted to JSON, you'll need to process recursively. I.e. if one of the fields of RecordA has type RecordB, it's of limited value to just put the RecordB object as a value in a dict, because the result still contains custom records. I wonder what the target use case is for this, and whether it should really be promising to "normalize" all of the data to only standard NS types (NSDictionary, NSArray, NSNumber).

}
case e: MExtern =>
if (e.objc.pointer) {
w.w(s"self.${idObjc.field(f.ident)}")
} else {
w.w(s"@(self.${idObjc.field(f.ident)})")
}
case _ => w.w(s"self.${idObjc.field(f.ident)}")
}

w.w(")")
}
}
w.wl("};")
}
w.wl

w.wl("@end")
})
}
Expand Down