Mike, Thank you for the input!
I think there are some outstanding issues with that (commend style checker), but I've just sent the change for review -- please take a look: https://gerrit.cloudera.org/#/c/3619/ Having this in place would allow to generate web-oriented documentation which we could be accessible after publishing it at the Kudu site. Meanwhile, I can deal with the outstanding minor issues in the background. Best regards, Alexey On Mon, Jul 11, 2016 at 3:31 PM, Mike Percy <mpe...@apache.org> wrote: > On Wed, Jul 6, 2016 at 12:56 PM, Alexey Serbin <aser...@cloudera.com> > wrote: > > > On Wed, Jul 6, 2016 at 11:36 AM, Todd Lipcon <t...@cloudera.com> wrote: > > > > > On Wed, Jul 6, 2016 at 11:22 AM, Alexey Serbin <aser...@cloudera.com> > > > wrote: > > > > > > > Todd, > > > > > > > > Thank you for the input! > > > > > > > > In the context of example you provided, would the following be > > > acceptable? > > > > > > > > /// @return the start time of the operation > > > > public Time GetStartTime() { > > > > return startTime; > > > > } > > > > > > > > I.e., just documenting the return type and not duplicating that in > the > > > > description? > > > > > > > > > > Personally I find that silly, too, since it's an inline function and > it's > > > typically obvious what a getter does. But, if people think it's useful, > > I'm > > > not going to fight about it :) > > > > I agree with Todd that this example is pretty useless. :) > > ... snip another code example with more parameters ... > > > In fact, IMO it actually has a negative impact on the readability of the > > > code, because I can fit less of the API on my screen at a time and I > have > > > to "skip over" the filler comments. > > > > > > This example comes from an internal-facing class though, and I can see > > the > > > argument for using the more verbose doxygen style for the public > > *exported* > > > API. To clarify, when you say "public APIs" do you mean "public" from a > > C++ > > > perspective? Or "public" from a "we will maintain this as an API for > > > consumers of the Kudu client" perspective? > > > > In this context that mean 'public from C++ perspective in files which we > > want to be processed by doxygen to auto-generate docs for customer-facing > > APIs' :) > > > > I.e., that's about public C++ members in files which we decide to be > > documented this way. At current state, that's about public C++ members > of > > classes/namespaces in src/kudu/client.h file only. > > > > This sounds good to me as a guideline, but I still think we should use our > own judgment. > > > > We can drop requirement to document all parameters and return > > type/value, > > > > but then we need to check for presence of valuable ones during > > > codereviews, > > > > no automation here. Is it ok? > > > > > > > > > > > I'm personally OK without automation, since I think comments are meant > > for > > > human consumption, and humans are probably the best judge of what a > good > > > description should be. In other words, even if doxygen verifies that a > > > function is "well commented", the reviewer still has to look at the > > comment > > > and make sure it actually makes sense, fully describes anything tricky, > > > etc. > > > > Exactly -- making sure the documentation have some value is on us and > that > > verification is to be done during code review. However, it's possible to > > introduce automatic verification of the _presence_ of corresponding > entries > > to catch issues with stale/absent items after re-factoring and adding a > new > > code. And that can be done automatically by doxygen. > > > > So, do you think it's worth adding that automated checks by doxygen just > > for _presence_ of documentation for parameters and return types/values? > > That implies every public method in client.h file must have > > parameters/return documented. :) > > > > No, I don't think we should enable a rule that all public methods must have > Doxygen, for exactly the reasons above. Since we're discussing only having > doxygen on a single header file, I don't think manual review would be > onerous. > > If we're in agreement, and to me it sounds like we're converging on that, > let's do a first pass on this on Gerrit. That will allow us to take a look > at the updated code together as well as the rendered doxygen HTML. I think > that will inform our opinions and is likely to lead to consensus on this > topic. > > Mike >
