Implement comments support

[4brunu]

Hi, Based on https://github.com/dropbox/djinni/issues/53 I implemented comments support, tests and readme documentation.

Now for adding code comments you add this to djinni files

// Code comment

Which will generate this in C++, Java and Objective-C

// Code comment

And for documentation comments you add this to djinni files

# Documentation comment

Which will generate this in C++, Java and Objective-C

/** Documentation comment */

There are two one limitation with code comments:

• Code comments have the same limitations that Documentation comments have in terms of where you can put them as described in https://github.com/dropbox/djinni/issues/53
• ~~Code comments and Documentation comments can be added next to each other, but I only was able to add support for one order. First Code comments and then Documentation comments, or vice versa, so I choose the order Code comments and them Documentation comments, but I can change that if you think that it makes more sense. Here is an example of what I was trying to explain: https://github.com/4brunu/djinni/blob/3e4c98a05326f9ef087159c4765fae6877564015/test-suite/generated-src/java/com/dropbox/djinni/test/ClientInterface.java#L11-L12~~

EDIT: In the last commits I basically redone this PR to simplify it, reduce the amount of changes introduced in the code base and improve its flexibility. EDIT2: Although this looks like a lot of changes, most of the changes are in generated files, to test the changes and the new code comments. The code changes are in ast.scala, parser.scala and generator.scala.

[4brunu]

@artwyman @xianwen Could you please take a look at this PR? Thanks

[xianwen]

Hi, @4brunu, heads up that I've just reverted this diff after discussion with @artwyman.

I just realized that the request in #53 was to ignore the non-doc comment, which is different from the intension of this diff. @artwyman will add more info later with his comment.

Let's have some further discussion regarding the design here. Thanks!

[artwyman]

Hey, @4brunu can you explain to me in more detail the intent of this PR? I'm a bit confused reading through changes, and given conflicts on the Python branch Xianwen has reverted until we can all get in sync on the right plan here.

If I understand right, the difference between doc comments an code comments in this diff is only way they're formatted in the output. They both still show up in the output, and need to be attached to an AST object, leading to the limitations in #53. What's the situation given that where you'd need to use a code comment rather than a doc comment? There's no hand-written implementation to describe, so it seems like all comments in the generated output should be documentation.

My expectation from past discussions (some of which were internal - sorry if you missed context) was that code comments in .djinni files wouldn't show up in the output at all, and would be a way of documenting the djinni implementation, not the generated files. As such, they wouldn't need to be attached to any sort of AST object but simply discarded by the parser. Indeed one implementation approach proposed by one of the original Djinni authors long ago would be to treat them as part of the whitespace pattern in the parser.

Let me know what you think given this context. I appreciate your taking on this feature regardless. Let's make sure we're all on the same page before this feature goes to any user who starts to depend on the behavior of // in ways which become hard to alter later.

[4brunu]

Hey @artwyman, oh I see, I didn't understand it that way, I thought the point was to be able to output code comments // and documentation comments /** */.

I need this, because of another tool I use that parses annotations in comments.

Would you be comfortable with introducing a flag that would control if the code comments would be added to the generated code or not? This way we would support both the use cases.

[xianwen]

Hi, @4brunu: Is your requirement A> simply change the format of the comment so that the tool you use can pick it up? or B> support 2 different kinds of comment for some special purpose?

If the requirement is A, then I'd suggest adding a command line argument for comment formatting, like --ident-java-field we have, to specify the way comments got formatted.

We think requirement B is unlikely, because if the comment is for the generated code itself, it should always be doc comment, as there is no implementation involved in the generated code, we only generate the interfaces. And if the comment is for the Djinni file itself, describing what happens within the Djinni file, then it should be ignored in the generation process and having no impact on the generated code.

If your requirement is indeed B, which is generating both the code comment and the doc comment in the resulting code, then could you please share more info so that we understand why it's needed?

Thanks!

[4brunu]

Hi @xianwen,

The requirement A would solve my problem, but also create another, because the tool would pick up the annotations, but I would lose the ability of generating documentation with djinni.

The ideal solution would be requirement B, because that way I could have the flexibility of generating annotations // annotation and also generating documentation /** doc */ at same time without have to choose between one or the other.

Network = interface +c {
        // annotation
        static create(): Network;
        # documentation
        sendToServer(text: string);
}

And to support your use-case of don't output the comments, it could be configurable with a flag.

[xianwen]

I see. So we're talking about 3 different kinds of comments here: 1> Doc comment for generated code, to be picked up by doc gen tools. 2> Annotations for generated code. 3> Comment for the Djinni file itself, like commenting out unused method, which it unrelated to the generated code itself.

We didn't have any requirement of case 2 in our usage of Djinni so far, only case 1 and case 3.

And case 2 and case 3 are for different purposes, if we want to support all 3 cases, we should use different syntax for all 3 cases. Because the intention of the comment should be clear at the time of writing the Djinni file.

Also, adding a flag wouldn't work. Because for case 2 and as it's implemented in this diff, the comments will be attached to a trailing identifier. So for a case like the following mentioned in #53, it still wouldn't work as there is no identifier to be attached to for # method_b();

blah = interface +c {
    method_a();
    # method_b();
}

Even though the other example in #53 as the following would work, it is still wrong to attach the comment # method_a(); to identifier method_b, as the intention of that comment wasn't to annotate method_b.

blah = interface +c {
    # method_a();
    method_b();
}

Our initial plan per #53 was to use # for case 1, and // for case 3. If we want to support case 2, we should probably think of a different syntax for it.

Thanks!

[artwyman]

Annotations are an interesting case I hadn't considered. Can you give more specifics on what kind of annotations you're generating for what sort of tool? What do you need to annotate: interfaces, records, field, methods, constants? Maybe that'll help illuminate possible compromises or workarounds here.

My worry about annotations is that they're likely to be quite language specific. E.g. if you're putting comments in your Djinni code to be parsed as annotations in C++, it doesn't really make sense to also propagate them to Java/ObjC/Python in the same way, since they all have their own way of doing annotations. For Java you might want an @AnnotationType similar to the "java-annotation" cmd-line option. A similar flag to that one that might be workable for your C++ annotations if they're all pretty uniform, as opposed to needing to differ on each individual method.

I seems awkward to have 3 different comment syntaxes, but that might be the only answer to support the most general use case. It might make more sense to come up with a more first-class concept for supporting annotations which could work in multiple languages, though, rather than treating them as a type of comment.

[4brunu]

I'm using https://github.com/krzysztofzablocki/Sourcery to generate https://github.com/dropbox/djinni/issues/15#issuecomment-209264813, but it can be used in other places, like generating mocks for testing proposes, etc.

Sourcery allows annotations // sourcery: Annotation everywhere, interfaces, records, field, methods, constants.

In an older version 0.10.1 they supported annotations with the documentation format /** sourcery: Annotation */, but in the current version 0.13.1 that format it's not supported, since the official annotation syntax is // sourcery: Annotation. I submitted a PR with support for documentation format and they accepted it, but I'm not sure if they will always support it, since it's not the oficial format, that's why I was trying to allow djinni to output comments like // comment.

I was looking for different comment syntax that we could use, but didn't find any option that would work in a good a backwards compatible way. What do you think of those options? Do you have any suggestion?

# djinni uses this is a documentation comment which output's /** this */ // common single line code comment /* common multi line code comment */ /** documentation comment */ <!-- XML comment --> -- SQL comment % Latex comment

[4brunu]

@artwyman @xianwen Do you have any suggestions to support booth annotations for generated code and comment for the Djinni file itself, like commenting out unused method, which it unrelated to the generated code itself?

[artwyman]

Sorry, I don't have a specific proposal, which is why I've left this PR fallow for so long. In theory it would be nice to support all options at once, but that's limited by the number of possible comment syntaxes, as well as the ability of developers to understand them. One idea which occurs to me would be an extra annotation after the comment syntax: e.g. #@cpp_annotation: <text goes here>

However at this point I think this is too large a feature to get merged into this repo, which is going to be going into inactive mode due to lack of maintainers at Dropbox. You're welcome to do whatever you want in your own fork, of course.

[4brunu]

What do you think of transfer this repo to some other company? Would dropbox be open to do that? If so, do you think this should be more visible? Maybe open an issue to discuss this?

[artwyman]

Let's talk about future meta-plans in Slack rather than in PR comments.