On Wed, Jul 6, 2016 at 11:22 AM, Alexey Serbin <[email protected]> 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?
>
> 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.
>
> Thanks,
>
> Alexey
>
> On Thu, Jun 30, 2016 at 4:05 PM, Todd Lipcon <[email protected]> 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 <[email protected]>
> > 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 <[email protected]> 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 <[email protected]
> >
> > >> 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 <[email protected]>
> > 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 <
> [email protected]
> > >
> > >> >> 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 <[email protected]>
> > >> 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 <
> > [email protected]
> > >> >
> > >> >> >> 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 <
> > >> >> >> > [email protected]> 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 <
> > >> >> >> [email protected]>
> > >> >> >> >> wrote:
> > >> >> >> >>
> > >> >> >> >> > +1 for the client facing APIs!
> > >> >> >> >> >
> > >> >> >> >> > Sameer Abhyankar
> > >> >> >> >> > Cloudera, Inc.
> > >> >> >> >> > (404) 431-7806
> > >> >> >> >> > [email protected]
> > >> >> >> >> >
> > >> >> >> >> >
> > >> >> >> >> >
> > >> >> >> >> > On Wed, Jun 8, 2016 at 10:25 PM, Mike Percy <
> > [email protected]
> > >> >
> > >> >> >> 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 <
> > >> [email protected]>
> > >> >> >> 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 <
> > >> >> >> [email protected]>
> > >> >> >> >> > > > 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
