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
>
