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

[korovkin]

Super useful to have this functionality for a few reasons:

1. easy to serialize the object to JSON or a file in order to send over HTTP or write to the file system respectively.
2. no need to comply with NSCoder complex protocol. if needed NSCoder implementation can be derived from the new toDict method very easily.
3. the code generated - (NSString *) description method can now just become: implementation to just: return [[self toDict] description]

please note, i am a complete n00b at Scala; First time see / code in Scala - any feedback would be highly appreciated.

[smarx]

Automated message from Dropbox CLA bot

@korovkin, thanks for the pull request! It looks like you haven't yet signed the Dropbox CLA. Please sign it here and update the thread so we can consider merging your code.

[korovkin]

so far have been adding the toDict manually to the generated djinni records, thus decided to code up the generator for it, so others could use it as well.

cheers !

[mknejp]

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]

yes, adding a nil value into a NSDictionary will throw an exception and crash.

[mknejp]

I'm not questioning the operation itself, but whether it has to implemented with a macro.

[korovkin]

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]

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

so can convert to a function or just unroll the code to be repeated, whatever you think is right

[mknejp]

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]

For the record, I support unrolling, as well as omitting the code where it's unnecessary (non-optional fields).

[mknejp]

so far have been adding the toDict manually to the generated djinni records, thus decided to code up the generator for it, so others could use it as well.

You can use record extenions (add +o to your djinni record definition) so you don't have to modify the autogenerated files. You them simply create your own ABCMyRecord class deriving from the generated ABCMyRecordBase and add your custom methods to that instead. All the code Djinni generates will accept/instantiate your ABCMyRecord.

[korovkin]

@mknejp the idea is for me not to write this code by hand as it's very simple for a machine to generate this code, not in ABCMyRecordBase and not in ABCMyRecord

[korovkin]

@smarx CLA signed

[korovkin]

example of generated code:

- (NSDictionary *)toDict
{
    #define _djinni_hide_null_(_o_) ((_o_)?(_o_):([NSNull null]))

    return @{@"__class_name__": [self.class description], @"firtName": _djinni_hide_null_(self.firtName), @"lastName": _djinni_hide_null_(self.lastName), @"emails": _djinni_hide_null_(self.emails), @"gender": _djinni_hide_null_(@(self.gender))};
}

existing description function:

- (NSString *)description
{
    return [NSString stringWithFormat:@"<%@ %p firtName:%@ lastName:%@ emails:%@ gender:%@>", self.class, self, self.firtName, self.lastName, self.emails, @(self.gender)];
}

[mknejp]

It'd be also great if you ran the test suite and added some new tests for the feature.

[steipete]

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

[artwyman]

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]

i have replied to the first question above, please take a look.

[steipete]

As the author of the description patch +1 the idea of then just calling toDict in the description implementation. Can you add that to the PR?

[korovkin]

@steipete yes, of course. i will add that too.

pardon my poor Scala skills :)

[artwyman]

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

[korovkin]

@artwyman i believe you are right, that was also the limitation of the original description method. Peter already mentioned that and is working on addressing that:

https://mobilecpp.slack.com/archives/djinni/p1446769200000039

[korovkin]

updated the PR as requested (making [description] a consumer of [toDict])
executed all the tests.

please take a look.

[artwyman]

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.

[korovkin]

guys, i feel like there are too many opinions here.
can we please assign one reviewer, as this is introducing a pretty significant amount of pain for such a small change.

[korovkin]

some of the questions as asked and answered more than once ... as it's difficult to navigate between different opinions / requests, some of which are a bit of 'nit pickish' while some are more fundamental.

who would like to be the reviewer for this?

[artwyman]

I'll take the role of designated reviewer here. Sorry for my part of the multi-comment confusion. I'm new to the github workflow, and was disoriented by its inability to carry over comments between multiple revisions. Similarly, I think mixing discussion of the feature design with discussion of code-level nitpicks has been unproductive.

I'd like to do a bit of a reset on this pull request, since i think this feature is turning out not to be as simple as it seemed originally. Let's start with a discussion of what the feature really is, and the use case you have in mind. Understanding that affects my thoughts on the importance of processing fields in a recursive way, as well as other aspects of the feature.

What is your use case for this feature? What should other Djinni users be able to do with this feature? How might it impact other Djinni users who don't need this feature?

A few thoughts of my own on those topics:

If JSON-compatible encoding is your intent, then lack of recursion seems like a serious limitation. It might be acceptable only for your use case because you don't happen to have any nested records, but a public feature of Djinni should be more general. JSON encoding seems like either its own feature (in a toJSON method), or a specialized form of "normalized representation". There are other standard ways of normalizing data in ObjC which should be considered, such as NSCoder (as you point out) or Key Value Coding (see NSKeyValueCoding).

If the intent is simply to provide a way to access the fields of a record based on a string key rather than a name, or iterate them, then that can be accomplished with a non-recursive method. I'd personally name that method (or property?) "fieldDict" to clarify its use. Without iteration, I think that's the situation for which Key Value Coding was designed so that might be worth considering as an alternative. NSKeyValueCoding provides dictionaryWithValuesForKeys: but not any way to enumerate all keys for iteration without already knowing which keys exist, so it may not solve the whole problem.

Another thing to consider in either case is support for non-NSObject external types, which may not have a known way to convert to something which can go into an NSDictionary, in which case another new YAML field would be required, which is already leading to some heavy discussion over in #160. If you don't want to tackle that, it would be best to make this feature opt-in, so that users with existing external types don't end up with non-compiling generated code after an update.

[artwyman]

Invoking code-review process now that it's available, and putting this in the original author's court until discussion topics are resolved.