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

Commit

Permalink
Follow naming convention for parameter names in method docstrings (#352)
Browse files Browse the repository at this point in the history
* Update parameter names in documentation

Because parameter names may not be written in the same format between
C++, Objective-C, and Java, it is impossible to have doxygen- or
javadoc-style @param annotations that match for all languages. This
change simply looks for "@param <PARAMNAME>" in the docstring and
rewrites it the same way as the sourcecode does.

* Replace all occurrences of parameter name in docstring

* Also update Java for static methods

* Add multi-word parameter name to test suite

And reference it from the docstring.

* Revert "Add multi-word parameter name to test suite"

This reverts commit 785a226.

* Add multi-word parameter name to test suite

And reference it from the docstring.
  • Loading branch information
louiswins authored and Xianwen Chen committed Feb 22, 2018
1 parent 565425d commit 0001a3e
Show file tree
Hide file tree
Showing 8 changed files with 32 additions and 4 deletions.
2 changes: 1 addition & 1 deletion src/source/CppGenerator.scala
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ class CppGenerator(spec: Spec) extends Generator(spec) {
// Methods
for (m <- i.methods) {
w.wl
writeDoc(w, m.doc)
writeMethodDoc(w, m, idCpp.local)
val ret = marshal.returnType(m.ret, methodNamesInScope)
val params = m.params.map(p => marshal.paramType(p.ty, methodNamesInScope) + " " + idCpp.local(p.ident))
if (m.static) {
Expand Down
4 changes: 2 additions & 2 deletions src/source/JavaGenerator.scala
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ class JavaGenerator(spec: Spec) extends Generator(spec) {
val throwException = spec.javaCppException.fold("")(" throws " + _)
for (m <- i.methods if !m.static) {
skipFirst { w.wl }
writeDoc(w, m.doc)
writeMethodDoc(w, m, idJava.local)
val ret = marshal.returnType(m.ret)
val params = m.params.map(p => {
val nullityAnnotation = marshal.nullityAnnotation(p.ty).map(_ + " ").getOrElse("")
Expand All @@ -158,7 +158,7 @@ class JavaGenerator(spec: Spec) extends Generator(spec) {
}
for (m <- i.methods if m.static) {
skipFirst { w.wl }
writeDoc(w, m.doc)
writeMethodDoc(w, m, idJava.local)
val ret = marshal.returnType(m.ret)
val params = m.params.map(p => {
val nullityAnnotation = marshal.nullityAnnotation(p.ty).map(_ + " ").getOrElse("")
Expand Down
2 changes: 1 addition & 1 deletion src/source/ObjcGenerator.scala
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ class ObjcGenerator(spec: Spec) extends BaseObjcGenerator(spec) {
if (i.ext.objc) w.wl(s"@protocol $self") else w.wl(s"@interface $self : NSObject")
for (m <- i.methods) {
w.wl
writeDoc(w, m.doc)
writeMethodDoc(w, m, idObjc.local)
writeObjcFuncDecl(m, w)
w.wl(";")
}
Expand Down
10 changes: 10 additions & 0 deletions src/source/generator.scala
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import djinni.syntax.Error
import djinni.writer.IndentWriter
import scala.language.implicitConversions
import scala.collection.mutable
import scala.util.matching.Regex

package object generatorTools {

Expand Down Expand Up @@ -416,6 +417,15 @@ abstract class Generator(spec: Spec)

// --------------------------------------------------------------------------

def writeMethodDoc(w: IndentWriter, method: Interface.Method, ident: IdentConverter) {
val paramReplacements = method.params.map(p => (s"\\b${Regex.quote(p.ident.name)}\\b", s"${ident(p.ident.name)}"))
val newDoc = Doc(method.doc.lines.map(l => {
paramReplacements.foldLeft(l)((line, rep) =>
line.replaceAll(rep._1, rep._2))
}))
writeDoc(w, newDoc)
}

def writeDoc(w: IndentWriter, doc: Doc) {
doc.lines.length match {
case 0 =>
Expand Down
3 changes: 3 additions & 0 deletions test-suite/djinni/varnames.djinni
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ _varname_record_ = record {
}

_varname_interface_ = interface +c {
# We should also rewrite parameter names in docstrings.
# _r_arg_ should be rewritten.
# _i_arg_ should not.
_rmethod_(_r_arg_: _varname_record_): _varname_record_;
_imethod_(_i_arg_: _varname_interface_): _varname_interface_;
}
5 changes: 5 additions & 0 deletions test-suite/generated-src/cpp/_varname_interface_.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ class VarnameInterface {
public:
virtual ~VarnameInterface() {}

/**
* We should also rewrite parameter names in docstrings.
* _r_arg_ should be rewritten.
* _i_arg_ should not.
*/
virtual VarnameRecord _rmethod_(const VarnameRecord & _r_arg_) = 0;

virtual std::shared_ptr<VarnameInterface> _imethod_(const std::shared_ptr<VarnameInterface> & _i_arg_) = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@
import javax.annotation.Nonnull;

public abstract class VarnameInterface {
/**
* We should also rewrite parameter names in docstrings.
* RArg should be rewritten.
* _i_arg_ should not.
*/
@Nonnull
public abstract VarnameRecord Rmethod(@Nonnull VarnameRecord RArg);

Expand Down
5 changes: 5 additions & 0 deletions test-suite/generated-src/objc/DBVarnameInterface.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@

@interface DBVarnameInterface : NSObject

/**
* We should also rewrite parameter names in docstrings.
* RArg should be rewritten.
* _i_arg_ should not.
*/
- (nonnull DBVarnameRecord *)Rmethod:(nonnull DBVarnameRecord *)RArg;

- (nullable DBVarnameInterface *)Imethod:(nullable DBVarnameInterface *)IArg;
Expand Down

4 comments on commit 0001a3e

@WoodyGuo
Copy link

@WoodyGuo WoodyGuo commented on 0001a3e May 14, 2018

Choose a reason for hiding this comment

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

Just encountered a problem after updating djinni, and it seems that it's caused by this commit.

Say, I have an interface defined as below:
`interface_xxx = interface +c {

const days_7: i8 = 7;

const days_14: i8 = 14;

const days_30: i8 = 30;

const days_default: i8 = days_30;

}`

Generated interface_xxx.h:
`class InterfaceXxx {
public:
virtual ~InterfaceXxx() {}

static constexpr int8_t DAYS_7 = 7;
static constexpr int8_t DAYS_14 = 14;
static constexpr int8_t DAYS_30 = 30;
static constexpr int8_t DAYS_DEFAULT;

}`

Generated interface_xxx.cc:

`namespace ns {

int8_t const InterfaceXxx::DAYS_7;
int8_t const InterfaceXxx::DAYS_14;
int8_t const InterfaceXxx::DAYS_30;
int8_t const InterfaceXxx::DAYS_DEFAULT;

}`

The compiler complained: Error:(28, 29) declaration of constexpr static data member 'DAYS_DEFAULT' requires an initializer.

@artwyman
Copy link
Contributor

Choose a reason for hiding this comment

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

@WoodyGuo are you sure it's this commit? This commit only modifies the documentation output, not the generation of enums, so that would be surprising.

Regardless, it would be helpful if you could log an issue for this, and provide a modified version of the test suite which demonstrates your issue, if possible.

@WoodyGuo
Copy link

Choose a reason for hiding this comment

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

Sorry I was mentioning the wrong commit. It must be that I was so sleepy as it was 2:00 AM in the morning. It should be Added support for constexpr for primitives in header files.

@WoodyGuo
Copy link

Choose a reason for hiding this comment

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

Filed #366

Please sign in to comment.