BTW - I concur that we are indeed in agreement ;) *Marco Massenzio* *Distributed Systems Engineer*
On Tue, Jul 14, 2015 at 11:08 PM, Marco Massenzio <[email protected]> wrote: > Hey Ben, > > my apologies - I expressed myself incorrectly: what I meant was a generic > "you random guy" (which I know, the correct way would have been to use > "one" and I didn't, my bad). > I promise never again to send an email at 8am, sleep-deprived and > oxygen-deprived in a crowded train. > > *Marco Massenzio* > *Distributed Systems Engineer* > > On Tue, Jul 14, 2015 at 1:54 PM, Benjamin Mahler < > [email protected]> wrote: > >> I'm bewildered by this reply, seems my comments were misinterpreted? >> >> I'm suggesting that we _do_ add doxygen comments to our libraries (stout, >> libprocess, state, cgroups, etc) as that is a nice way to make them >> accessible. I'm less convinced that there's value in adding doxygen in >> _every_ internal header in mesos (e.g. master / slave, as these are not >> libraries). >> >> Also, folks _are_ free to add documentation no matter what, I'm just >> suggesting that they keep the style consistent within a library header. >> They are free to do a conversion sweep before _or_ after, it does not >> block >> their ability to contribute to documentation, just means they need to >> maintain consistent formatting. >> >> Sad to see implications that I do not care about productivity, or that I'm >> trying to prevent people from contributing documentation... :( >> >> On Tue, Jul 14, 2015 at 8:12 AM, Marco Massenzio <[email protected]> >> wrote: >> >> > On Mon, Jul 13, 2015 at 9:41 PM, Benjamin Mahler < >> > [email protected]> >> > wrote: >> > >> > > Let me try to contain the length of this thread, two points don't >> seem to >> > > agree my request and benh's reply. >> > > >> > > (1) You're saying all non-trivial classes / methods in headers should >> > have >> > > javadoc, whereas benh is saying APIs. Are these the same? I'd much >> rather >> > > see this focused on APIs (i.e. libraries) rather than internal >> > > implementations (e.g. master / slave) since people operating within >> the >> > > latter ideally should be comfortable reading the code. Library users, >> > less >> > > so. >> > > >> > >> > I find difficult to follow the reasoning here: are you suggesting that >> > every time a developer uses a "library" function they are supposed to >> > reverse engineer the code? >> > that feels not a very efficient way of running a large development team >> and >> > it certainly was not the way we rolled at Google :) >> > >> > On the contrary, due to their frequent and extensive use, IMO library >> > methods/classes ought to have _extensive_ documentation. >> > >> > Then again, maybe it's just me caring about productivity in my teams... >> > >> > >> > > >> > > (2) Doing the incremental change will make things inconsistent :( >> Given >> > > that doing a javadoc conversion sweep for a library header is not that >> > > tedious, it seems wise to just have consistency as a forcing function >> for >> > > folks to do sweeps. Plus we keep the documentation consistently >> > formatted, >> > > which seems like a big win! >> > > >> > >> > Sure - and +100 to that! >> > But, in the meantime, let's not have folks *not* add javadoc (or worse, >> > demand they remove those they may have already added during code >> review!) >> > or require them to do a "sweep" only because they added ONE method and >> want >> > to document that. >> > >> > Again, I'm trying here to lower the bar for participation for folks who >> are >> > not (yet) deep experts in Mesos' internals and encourage participation >> of >> > people who are excited about contributing functionality to Mesos: if the >> > cost is to have to "reverse engineer"[*] some obscure parts of >> libprocess >> > and spend days (or weeks) trying to figure out how to correctly use the >> > base libraries, I think we'll all lose in the long run. >> > >> > Bottom line, Ben - if you don't feel like adding documentation/javadoc >> to >> > the methods/classes you contribute, I guess that's fine: but, please, >> let's >> > not prevent folks from doing so, that's all I'm saying. >> > >> > Thanks! >> > >> > [*] NOTE - I still expect people to intimately understand the >> functionality >> > of libprocess/stout and whatever else is already in Mesos proper: >> however, >> > that would ideally be gained by studying extensive documentation; >> looking >> > at existing and sample code: and experimenting with it. >> > What I do object to is the extra layer of effort in having to >> > reverse-engineer large, undocumented and complex areas of the code. >> > >> > >> > > >> > > >> > > On Fri, Jul 10, 2015 at 9:39 AM, Marco Massenzio <[email protected] >> > >> > > wrote: >> > > >> > > > On Thu, Jul 9, 2015 at 5:23 PM, Benjamin Mahler < >> > > [email protected] >> > > > > >> > > > wrote: >> > > > >> > > > > A couple of thoughts: >> > > > > >> > > > > (1) When introducing javadoc comments, can we please keep comment >> > style >> > > > > consistent within files and APIs? For the most part, it seems >> folks >> > are >> > > > > introducing javadoc in consistent sweeps, which is great. >> However, it >> > > > looks >> > > > > also like there are reviews and commits where we are introducing >> > > javadoc >> > > > + >> > > > > non-javadoc within a file / api, would love to avoid the >> > inconsistency. >> > > > :( >> > > > > >> > > > >> > > > This is a great suggestion, and I am really excited to see people >> doing >> > > > this and helping us having a great, well-documented codebase! >> > > > >> > > > Until we get to the point where the majority of the codebase is well >> > > > documented, I would suggest we use what in past similar situations I >> > > called >> > > > "the ratchet" concept: whatever is added must be Done Right, and you >> > can >> > > > never slip back. >> > > > This will, in due course, get us all where we want to be, without >> > slowing >> > > > progress too much. >> > > > >> > > > (Am I correct in assuming you too were *not* suggesting that, if we >> add >> > > 1-2 >> > > > methods with javadoc-style docs, *all* existing ones must be updated >> > too, >> > > > right?) >> > > > >> > > > >> > > > > (2) Where are we planning to introduce javadoc comments? APIs >> only? >> > All >> > > > > headers? Would love to see some communication around how we'd like >> > > folks >> > > > to >> > > > > be proceeding. Maybe I missed it, but can't seem to find an email >> > with >> > > > > this. >> > > > > >> > > > >> > > > The idea would be to have javadoc-style Doxygen comments for all >> header >> > > > files, for all *non-trivial* public classes/methods - initially, >> this >> > > will >> > > > be a *requirement* only for newly added code, with the occasional >> > "sweep" >> > > > on existing classes; hopefully, we'll eventually get to the point >> where >> > > the >> > > > "undocumented wilderness" footprint has shrunk to the point where we >> > can >> > > > mandate complete compliance. >> > > > >> > > > I think it would also be great to encourage "drive-by" additions: >> it's >> > > > often the case that one spends time trying to figure out how an (as >> > yet, >> > > > undocumented) API/method works while they are using it in other >> parts >> > of >> > > > the code, and it would be a shame to waste that effort. >> > > > If that's done in a "chained" patch, so much the better, but the >> > "admin" >> > > > burden is sometimes not worth the effort: again, I'd like to >> encourage >> > > > folks to add as much docs as they feel like doing, by lowering the >> > > barriers >> > > > to doing so. >> > > > >> > > > >> > > > > (3) I ask because there is a tradeoff: we make the code more >> verbose >> > to >> > > > > navigate visually. Also, sometimes we document things >> unnecessarily: >> > > > > >> > > > >> > > > Couldn't agree more! >> > > > That was the "non-trivial" part of my comment above :) >> > > > >> > > > >> > > > > /** >> > > > > * Sends a message with data without a return address. >> > > > > * >> > > > > * @param to Receiver of the message. >> > > > > * @param name Name of the message. >> > > > > * @param data Data to send (gets copied). >> > > > > * @param length Length of data. >> > > > > */ >> > > > > void post(const UPID& to, >> > > > > const std::string& name, >> > > > > const char* data = NULL, >> > > > > size_t length = 0); >> > > > > >> > > > > Here, having a 'to' or 'receiver' as a variable name is pretty >> > > > > self-evident, ditto for 'messageName', 'data', 'length'. Are we ok >> > with >> > > > > omitting these kinds of comments? It seems like we have to be >> asking >> > > > > ourselves when this provides value. Thoughts? >> > > > > >> > > > >> > > > +1 >> > > > >> > > > Thanks for raising the issue, Ben - and sorry for not doing this >> > before: >> > > I >> > > > got over-enthusiastic about having great documented code :) >> > > > >> > > >> > >> > >
