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 :) > > > > > > As Adar mentioned, it's worth having some constraints for automatic > > validation, otherwise we are about to check both for presence and > > semantic/clarifying value of those comments during code reviews. The > > former can be automated if having requirement for documenting all return > > types and parameters. > > > > Out-of-the-box doxygen can automatically check for presence of comments > on > > public methods (except for constructors and destructors) and, as > additional > > option, that provided documentation have all parameters and return types > > covered. From this perspective, having return type/value documented in > the > > snippet above is better than having description but no doc on return > > type/value. > > > > Sure, but to take another example (from tablet.h): > > // Create a new row iterator for some historical snapshot. > Status NewRowIterator(const Schema &projection, > const MvccSnapshot &snap, > const OrderMode order, > gscoped_ptr<RowwiseIterator> *iter) const; > > To me, it doesn't add any value to rewrite this comment as: > /// Create a new row iterator for some historical snapshot. > /// @param projection the projection > /// @param snap the snapshot > /// @param order the order mode (see OrderMode enum) > /// @param [out] iter the returned iterator > Status NewRowIterator(const Schema &projection, > const MvccSnapshot &snap, > const OrderMode order, > gscoped_ptr<RowwiseIterator> *iter) const; > > 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. > > > > > > 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. :) As an option, we can require presence of some comment for a method/member, and no checks for parameters/returns. That check can be performed automatically by doxygen. BTW, styling of those comments (like spacing, etc.) is a separate thing, and that is not handled by doxygen. For that it might required to run some style-checker tool (vera++, cppcheck, cpplint, etc.) but none of those I checked has something ready to use in their stock distribution. I would suggest to go without those styles first, but add them later (e.g., we could implement those for vera++ as custom rules in TCL). > > > > > > Thanks, > > > > Alexey > > > > On Thu, Jun 30, 2016 at 4:05 PM, Todd Lipcon <t...@cloudera.com> wrote: > > > > > Sure, I'm +1 on having this for public-facing headers. > > > > > > That said, I don't think we should go overboard. In particular, it > always > > > bugs me to see JavaDoc or Doxygen like: > > > > > > /// Gets the start time of the operation. > > > /// @return the start time of the operation > > > public Time GetStartTime() { > > > return startTime; > > > } > > > > > > In other words, I'm only in favor of docs that actually add real > > clarifying > > > value, rather than being an English restatement of the code. Given > that, > > > I'm -1 on enforcing 'all parameters and return types should be > > doxygened'. > > > > > > -Todd > > > > > > > > > On Tue, Jun 28, 2016 at 2:42 PM, Alexey Serbin <aser...@cloudera.com> > > > wrote: > > > > > > > Adar, > > > > > > > > Thank you for the analysis -- I'm glad you found it feasible to adopt > > the > > > > doxygen-like style for the header files at least :) > > > > > > > > There isn't 'standard' doxygen style -- it's just a set > > doxygen-specific > > > > keywords and formatting to allow doxygen to generate the docs (you > can > > > find > > > > more info on those at > > > > http://www.stack.nl/~dimitri/doxygen/manual/commands.html). From > our > > > > side, that's more about choosing a set of conventions on _what_ we > want > > > to > > > > document and about _style_ of those comments (like using this or that > > > > commenting style, indenting multi-line comments, specifying a brief > > > > description for every public member and having detailed description > > > > optional, mandatory documentation for parameters and return values, > > > etc.). > > > > > > > > As for the enforcement of the doxygen-like doc rules, I can see the > > > > following: > > > > * The doxygen tool itself generates warnings on non-documented member > > > > functions or non-documented parameters/return values, so that's easy > to > > > > catch -- just make run of the doxygen tool as one of the build steps > > > (like > > > > 'make doxygen-docs' or alike). > > > > * The style for the doxygen comments: I need to make some research on > > > > automating enforcement of that. Hopefully, we could have a custom > > config > > > > for vera++ or other C++ style checker to do that. I'll take a look > at > > > that > > > > and report on my findings. I think I could get some results here > this > > or > > > > next week. > > > > > > > > BTW, if you want to see the auto-generated documentation, save the > > > > attached files at the same empty directory and run 'doxygen > > > > kudu_client.cfg' from that dir (on Mac, you could install doxygen via > > > > MacPorts or HomeBrew if not yet installed). Then, to browse the > > results, > > > > just open /tmp/kudu_docs/html/index.html in your favorite browser: > you > > > > would be interested to click through the tabs and spend more time at > > the > > > > 'Data Structures' and its sub-tabs. > > > > > > > > > > > > Thanks, > > > > > > > > Alexey > > > > > > > > On Tue, Jun 28, 2016 at 1:24 PM, Adar Dembo <a...@cloudera.com> > wrote: > > > > > > > >> Thanks for putting this together. I skimmed it and have a couple > > > thoughts: > > > >> - Overall, this is less overhead than I thought it would be. I'm > happy > > > >> about that. > > > >> - With the exception of annotating @param and @return correctly, it > > > >> seems like it won't be much work to adapt our current comments to > this > > > >> style. > > > >> - I especially like the annotations for in and out parameters. > That's > > > >> useful information that we don't communicate well (if at all) today, > > > >> so it's definitely an improvement. > > > >> - How much (if at all) does this deviate from "standard" doxygen > > style? > > > >> - I think it's really important to have a build-time checkstyle to > > > >> enforce this. Otherwise, either code reviews would become a drag or > > > >> our adherence to the syntax would deteriorate. Do you have any > > > >> thoughts on how we could do this? > > > >> > > > >> > > > >> On Tue, Jun 28, 2016 at 10:59 AM, Alexey Serbin < > aser...@cloudera.com > > > > > > >> wrote: > > > >> > Hi, > > > >> > > > > >> > I re-formatted documentation in client.h file to be in > > doxygen-style. > > > >> > Please take a look at it to get an idea on the suggested style, > > etc.: > > > >> > > > > https://gist.github.com/alexeyserbin/faad98b2ec9959acdf7256ff7d0bf139 > > > >> > I also attached the file as is. > > > >> > > > > >> > Sure, this is just an initial draft -- let's communicate on > > different > > > >> > aspects and converge on the style conventions. > > > >> > > > > >> > Please let me know if you see there is something worth changing. > > Your > > > >> > feedback is highly appreciated! > > > >> > > > > >> > > > > >> > Thanks, > > > >> > > > > >> > Alexey > > > >> > > > > >> > On Thu, Jun 9, 2016 at 12:01 PM, Adar Dembo <a...@cloudera.com> > > > wrote: > > > >> >> > > > >> >> We don't enforce much semantic consistency on our comments today. > > We > > > >> >> follow the Google style guide, and cpplint.py (tied to our 'make > > > lint' > > > >> >> target) can enforce some stuff, but I don't think it does much, > > > though > > > >> >> I'm not sure; I haven't really played around with it. So yeah, if > > we > > > >> >> enforce anything it's during code review, and that turns out to > be > > > >> >> tedious and time consuming for everyone involved. Take a look at > > > >> >> cpplint.py; maybe it's got some checks that we're not running > > that'd > > > >> >> yield better consistency. > > > >> >> > > > >> >> > > > >> >> > > > >> >> > > > >> >> On Thu, Jun 9, 2016 at 11:51 AM, Alexey Serbin < > > aser...@cloudera.com > > > > > > > >> >> wrote: > > > >> >> > Adar, > > > >> >> > > > > >> >> > I concur -- noisy and useless comments is an issue if not taken > > > care > > > >> of. > > > >> >> > > > > >> >> > BTW, what about current comments in the code -- how is semantic > > > >> >> > consistency > > > >> >> > being enforced? I would think it's more about code-review > time; > > > >> would > > > >> >> > not > > > >> >> > expect much automation there. As for the style, I also think > > > >> >> > style-checker > > > >> >> > would be great to have. Let's see what I can find out there. > I > > > >> >> > remember > > > >> >> > there were some tools (vera++, etc.). From that regard I > really > > > >> like Go > > > >> >> > lang approach on this :) > > > >> >> > > > > >> >> > Let me provide a snippet/sample of how that doxygen-styled > > comments > > > >> >> > would > > > >> >> > look like with current code and we can start from that. > > > >> >> > > > > >> >> > > > > >> >> > Thanks, > > > >> >> > > > > >> >> > Alexey > > > >> >> > > > > >> >> > > > > >> >> > > > > >> >> > On Thu, Jun 9, 2016 at 10:21 AM, Adar Dembo <a...@cloudera.com > > > > > >> wrote: > > > >> >> > > > > >> >> >> I agree with Mike. In my experience Doxygen-style > documentation > > > can > > > >> be > > > >> >> >> very noisy in that it forces developers to write reams upon > > reams > > > of > > > >> >> >> "obvious" documentation (i.e. adding a line for a @return that > > is > > > >> >> >> completely self-explanatory). > > > >> >> >> > > > >> >> >> I'm open to the idea of using it for public headers provided: > > > >> >> >> - It's easy to draw the line between those files and the rest > of > > > the > > > >> >> >> codebase. > > > >> >> >> - We have some script to automate checking the Doxygen style > on > > > >> those > > > >> >> >> files. > > > >> >> >> > > > >> >> >> I'd also like us spend some time discussing which aspects of > > > Doxygen > > > >> >> >> are worth implementing and which are excessive. Alexey, could > > you > > > >> >> >> drive this discussion? Not sure if it makes more sense to do > it > > > >> over a > > > >> >> >> sample patch (to e.g. client.h) or in a vacuum, whatever you > > think > > > >> is > > > >> >> >> best. > > > >> >> >> > > > >> >> >> On Thu, Jun 9, 2016 at 9:57 AM, Alexey Serbin < > > > aser...@cloudera.com > > > >> > > > > >> >> >> wrote: > > > >> >> >> > Hi All, > > > >> >> >> > > > > >> >> >> > Thank you for sharing your thoughts on this. > > > >> >> >> > > > > >> >> >> > From what I see the consensus is that for client API C++ > > headers > > > >> it > > > >> >> >> > makes > > > >> >> >> > sense to re-format in-code documentation to be in doxygen > > style. > > > >> So, > > > >> >> >> > I'm > > > >> >> >> > thinking about discussing style-related questions with Mike > > > Percy > > > >> and > > > >> >> >> > others, preparing a patch and sending it for review. > > > >> >> >> > > > > >> >> >> > I think it's worth publishing the auto-generated results > > (HTML) > > > >> along > > > >> >> >> with > > > >> >> >> > client Java API docs. Misty, what help is needed there? > > > >> >> >> > > > > >> >> >> > > > > >> >> >> > Thanks, > > > >> >> >> > > > > >> >> >> > Alexey > > > >> >> >> > > > > >> >> >> > On Thu, Jun 9, 2016 at 9:37 AM, Misty Stanley-Jones < > > > >> >> >> > mstanleyjo...@cloudera.com> wrote: > > > >> >> >> > > > > >> >> >> >> I'm +1 too. Do we want to publish these as HTML like we do > > with > > > >> >> >> >> Javadoc > > > >> >> >> >> too? If so, who wants to volunteer to help me figure that > > out? > > > >> >> >> >> > > > >> >> >> >> On Thu, Jun 9, 2016 at 8:28 AM, Sameer Abhyankar < > > > >> >> >> sabhyan...@cloudera.com> > > > >> >> >> >> wrote: > > > >> >> >> >> > > > >> >> >> >> > +1 for the client facing APIs! > > > >> >> >> >> > > > > >> >> >> >> > Sameer Abhyankar > > > >> >> >> >> > Cloudera, Inc. > > > >> >> >> >> > (404) 431-7806 > > > >> >> >> >> > sam...@cloudera.com > > > >> >> >> >> > > > > >> >> >> >> > > > > >> >> >> >> > > > > >> >> >> >> > On Wed, Jun 8, 2016 at 10:25 PM, Mike Percy < > > > mpe...@apache.org > > > >> > > > > >> >> >> wrote: > > > >> >> >> >> > > > > >> >> >> >> > > Hey Alexey, > > > >> >> >> >> > > Glad you brought it up. I'm inclined to agree that this > > > >> would be > > > >> >> >> >> helpful > > > >> >> >> >> > > for users of the C++ client APIs. > > > >> >> >> >> > > > > > >> >> >> >> > > But... I'm hesitant to do it for the rest of the code > > base. > > > >> It > > > >> >> >> >> > > can > > > >> >> >> be > > > >> >> >> >> > > pretty noisy, requires ongoing maintenance, and > honestly > > I > > > >> don't > > > >> >> >> think > > > >> >> >> >> it > > > >> >> >> >> > > is helpful if you have decent dev tools set up (good > > editor > > > >> with > > > >> >> >> good > > > >> >> >> >> > > plugins, ack / ag, etc) > > > >> >> >> >> > > > > > >> >> >> >> > > If we adopt Doxygen, I think we should agree on a > > > particular > > > >> >> >> >> > > style > > > >> >> >> and > > > >> >> >> >> > > adhere to it. > > > >> >> >> >> > > > > > >> >> >> >> > > Mike > > > >> >> >> >> > > > > > >> >> >> >> > > > > > >> >> >> >> > > On Wed, Jun 8, 2016 at 6:00 PM, Dan Burkert < > > > >> d...@cloudera.com> > > > >> >> >> wrote: > > > >> >> >> >> > > > > > >> >> >> >> > > > +1 from me for at least doing it in client.h and the > > few > > > >> other > > > >> >> >> public > > > >> >> >> >> > > > header files. These files have pretty much settled > > down, > > > >> so > > > >> >> >> >> > > > it > > > >> >> >> >> > shouldn't > > > >> >> >> >> > > > be too onerous to keep the doxygen comments up to > date > > > once > > > >> >> >> >> > > > they > > > >> >> >> are > > > >> >> >> >> > > > created. > > > >> >> >> >> > > > > > > >> >> >> >> > > > - Dan > > > >> >> >> >> > > > > > > >> >> >> >> > > > On Wed, Jun 8, 2016 at 4:36 PM, Alexey Serbin < > > > >> >> >> aser...@cloudera.com> > > > >> >> >> >> > > > wrote: > > > >> >> >> >> > > > > > > >> >> >> >> > > > > Hi All, > > > >> >> >> >> > > > > > > > >> >> >> >> > > > > Looking at current documentation for Kudu client > API > > at > > > >> >> >> >> getkudu.org > > > >> >> >> >> > > > > you can see that Java API has auto-generated > > > >> documentation > > > >> >> >> >> > > > > but C++ API does not. > > > >> >> >> >> > > > > > > > >> >> >> >> > > > > Adding doxygen-style comments into C++ header files > > > would > > > >> >> >> >> > > > > allow to auto-generate API documentation for C++ > > client > > > >> API > > > >> >> >> >> > > > > as > > > >> >> >> >> well. > > > >> >> >> >> > > > > > > > >> >> >> >> > > > > Assuming doxygen-formatted comments are not > > considered > > > >> >> >> >> > > > > as a violation of current C++ coding style, > > > >> >> >> >> > > > > would it make sense to introduce doxygen-style > > comments > > > >> >> >> >> > > > > for C++ client API headers? > > > >> >> >> >> > > > > > > > >> >> >> >> > > > > If it's worth it, in-line documentation in other > > parts > > > of > > > >> >> >> >> > > > > the > > > >> >> >> C++ > > > >> >> >> >> > code > > > >> >> >> >> > > > base > > > >> >> >> >> > > > > could be re-formatted into doxygen style as well. > > > >> >> >> >> > > > > > > > >> >> >> >> > > > > What do you think? > > > >> >> >> >> > > > > > > > >> >> >> >> > > > > Thank you! > > > >> >> >> >> > > > > > > > >> >> >> >> > > > > > > > >> >> >> >> > > > > Best regards, > > > >> >> >> >> > > > > > > > >> >> >> >> > > > > Alexey > > > >> >> >> >> > > > > > > > >> >> >> >> > > > > > > >> >> >> >> > > > > > >> >> >> >> > > > > >> >> >> >> > > > >> >> >> > > > >> > > > > >> > > > > >> > > > > > > > > > > > > > > > > > -- > > > Todd Lipcon > > > Software Engineer, Cloudera > > > > > > > > > -- > Todd Lipcon > Software Engineer, Cloudera >