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

Follow naming convention for parameter names in method docstrings #352

Merged
merged 6 commits into from
Feb 22, 2018

Conversation

louiswins
Copy link
Contributor

The problem here is that if I have differing naming conventions in different languages then if I mention a method parameter name in my djinni docstring it will be wrong in at least one language (e.g. some_parameter in C++ but someParameter in Obj-C and Java). And I will probably mention them if I'm using doxygen or javadoc (@param some_parameter). Beyond being annoying, this causes warnings in XCode and probably other environments.

The fix will run every method docstring through a series of regexes that replace each djinni parameter name with the corresponding name in the language naming convention.

Drawbacks

  • I don't try to parse doxygen semantics or anything, I just blindly replace. But I don't expect that there are many cases where a parameter name with an underscore is used in a method docstring to mean something that is not actually the parameter name.
  • I don't replace non-parameter names, so if you refer to a different method in your docstring it won't be rewritten.

This is a fix for #332, which I opened.

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.
@artwyman
Copy link
Contributor

Thanks for taking a stab at this. Sounds good in abstract. I haven't looked at the code in detail.

You should add some cases which trigger the new behavior in the test suite and/or example app, then re-run the generators so that we can see the generated output changes in this diff.

@louiswins
Copy link
Contributor Author

Will do

Copy link

@xianwen xianwen left a comment

Choose a reason for hiding this comment

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

LGTM, will merge after testing

@xianwen xianwen merged commit 0001a3e into dropbox:master Feb 22, 2018
KhalilBellakrid pushed a commit to KhalilBellakrid/djinni that referenced this pull request Mar 8, 2018
…opbox#352)

* 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.
danielkaldheim pushed a commit to danielkaldheim/djinni that referenced this pull request Mar 27, 2018
…opbox#352)

* 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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants