This has come up in a couple of reviews, seems like we should add some soft guidelines around how to format comments for readability.
In particular, the reason that we wrapped at 70 in the past was for readability, so it would be great to continue doing so as a soft stylistic rule. The other thing we've been doing for readability is reducing "jaggedness" (variability in line lengths). It would be great to establish these as soft rules and encourage new contributors / committers to follow them. Compare these two comments in Master::updateTask. The first one wraps at 70 and reduces jagedness, the second wraps at 80 and is more jagged: https://github.com/apache/mesos/blob/0.25.0/src/master/master.cpp#L6057 https://github.com/apache/mesos/blob/0.25.0/src/master/master.cpp#L6072 I can provide more examples to help clarify. If no one objects, I'll follow up with an update to the style guide. Thoughts appreciated! On Thu, Sep 10, 2015 at 8:59 AM, Bernd Mathiske <be...@mesosphere.io> wrote: > +1 > > On Sep 10, 2015, at 4:21 PM, tommy xiao <xia...@gmail.com> wrote: > > > > +1 > > > > 2015-09-10 9:44 GMT+08:00 Marco Massenzio <ma...@mesosphere.io>: > > > >> +1 > >> > >> > >> > >> > >> Thanks, Michael! > >> > >> > >> > >> — > >> Sent from my iPhone, which is not as good as you'd hope to fix trypos n > >> abbrvtn. > >> > >> On Wed, Sep 9, 2015 at 6:23 PM, Michael Park <mcyp...@gmail.com> wrote: > >> > >>> I've removed the 70 column restriction on comments from the style > guide: > >>> > >> > https://github.com/apache/mesos/commit/f9c2604ea97b91f8a9ec3b2863317761679b1c86 > >>> Also, based on the comments, it seems like we should allow 80 column > >>> comments but omit the sweeping change. > >>> Thanks, > >>> MPark. > >>> On Wed, Aug 12, 2015 at 6:13 PM Marco Massenzio <ma...@mesosphere.io> > >> wrote: > >>>> On Wed, Aug 12, 2015 at 4:09 AM, Bernd Mathiske <be...@mesosphere.io> > >>>> wrote: > >>>> > >>>>> Like BenM, > >>>>> > >>>>> +1 on allowing 80 column comments > >>>>> > >>>> +1 > >>>> (it really IS annoying having to keep an eye on the bottom column > >> counter > >>>> when typing comments :) > >>>> > >>>> > >>>>> -1 on sweeping changes; incremental changes when touching old > comments > >>>>> will do IMHO > >>>>> > >>>>> +1 on the -1? :) > >>>> Incremental changes are good and I doubt anyone will be "confused" by > >> them. > >>>> > >>>> > >>>>> Bernd > >>>>> > >>>>>> On Aug 12, 2015, at 12:51 AM, Michael Park <mcyp...@gmail.com> > >> wrote: > >>>>>> > >>>>>> Ben, thanks for your input! > >>>>>> > >>>>>> Another update on this topic: the patches around break before braces > >>>> for > >>>>>> *enum* style and overloaded operators have been committed. > >>>>>> > >>>>>> On Tue, Aug 11, 2015 at 6:23 PM Benjamin Mahler < > >>>>> benjamin.mah...@gmail.com> > >>>>>> wrote: > >>>>>> > >>>>>>> We already don't necessarily wrap at 70 characters (often we wrap > >>>>> before 70 > >>>>>>> to reduce "jaggedness" or to make it look cleaner). > >>>>>>> > >>>>>>> So with the change to 80, this still makes all existing comments > >>>> valid. > >>>>> We > >>>>>>> can still encourage folks to write paragraphs in a way that is > >> easy to > >>>>>>> digest for the reader. That is, I think we should still be trying > >> not > >>>> to > >>>>>>> write jagged paragraphs of comments, it's just not a hard stylistic > >>>>>>> violation given we don't have an algorithm for this. > >>>>>>> > >>>>>>> So +1 to relaxing the hard 70 character rule, but -1 to sweeping > >>>> across > >>>>> all > >>>>>>> the comments or doing wrapping based only on line length rather > >> than > >>>>>>> jaggedness going forward. > >>>>>>> > >>>>>>> On Sat, Aug 8, 2015 at 3:25 PM, Joris Van Remoortere < > >>>>> jo...@mesosphere.io> > >>>>>>> wrote: > >>>>>>> > >>>>>>>> I will volunteer to update all the comments to wrap at 80 if we > >> agree > >>>>> to > >>>>>>>> keep the code base consistent. > >>>>>>>> Naturally that is also my vote ;-) > >>>>>>>> Joris > >>>>>>>> > >>>>>>>>> On Aug 8, 2015, at 1:40 PM, Michael Park <mcyp...@gmail.com> > >> wrote: > >>>>>>>>> > >>>>>>>>> An update on this topic since we covered it at the community > >>>> developer > >>>>>>>> sync. > >>>>>>>>> > >>>>>>>>> 1. We will adopt *Mozilla*'s *BreakBeforeBraces* style as their > >>>> style > >>>>>>>> is > >>>>>>>>> equivalent to ours. The only change this entails for our > >> codebase > >>>> is > >>>>>>> to > >>>>>>>>> consistently wrap the braces for *enum* definitions, as we're > >>>>>>> currently > >>>>>>>>> inconsistent. I've taken on the work involved in this change: > >>>>>>>>> - stout: https://reviews.apache.org/r/37258 > >>>>>>>>> - libprocess: https://reviews.apache.org/r/37259 > >>>>>>>>> - mesos: https://reviews.apache.org/r/37260 > >>>>>>>>> 2. We will drop the rule for adding spaces around overloaded > >>>>>>>>> operators. We'll simply do a sweep of the codebase to update > >> all of > >>>>>>>> them > >>>>>>>>> consistently. Artem has kindly taken action on this already: > >>>>>>>>> - stout: https://reviews.apache.org/r/37018/ > >>>>>>>>> - libprocess: https://reviews.apache.org/r/37017/ > >>>>>>>>> - mesos: https://reviews.apache.org/r/37013/ > >>>>>>>>> 3. We will drop the rule for wrapping comments at 70 > >> characters. > >>>>>>> We > >>>>>>>>> have a few options to proceed here: > >>>>>>>>> - Keep all the existing comments in tact, and simply allow > >> new > >>>>>>>>> comments to wrap at 80, this is less work. > >>>>>>>>> - Update all instances of the comments wrapping at 70 to be > >>>>>>> wrapped > >>>>>>>>> at 80, so that we can be consistent. > >>>>>>>>> > >>>>>>>>> I proposed that we simply allow new comments to wrap at 80, but I > >>>> have > >>>>>>>>> heard arguments to update the existing comments, so that we can > >> be > >>>>>>>>> consistent across the codebase. If you have a suggestion/opinion > >> on > >>>>> how > >>>>>>>> we > >>>>>>>>> should proceed with (3), please share! > >>>>>>>>> > >>>>>>>>> Thanks, > >>>>>>>>> > >>>>>>>>> MPark. > >>>>>>>>> > >>>>>>>>> On Mon, Aug 3, 2015 at 2:01 PM Alexander Rojas < > >>>>>>> alexan...@mesosphere.io> > >>>>>>>>> wrote: > >>>>>>>>> > >>>>>>>>>> I also vote up for that! I rather change our guidelines a little > >>>> bit > >>>>>>>> than > >>>>>>>>>> waiting for months > >>>>>>>>>> to get our changes into the clang-format source without any > >>>> security > >>>>>>>> that > >>>>>>>>>> it will actually land. > >>>>>>>>>> > >>>>>>>>>>> On 31 Jul 2015, at 09:31, Alex Rukletsov <a...@mesosphere.com> > >>>>>>> wrote: > >>>>>>>>>>> > >>>>>>>>>>> I think automation is very important. If we should slightly > >> change > >>>>>>> our > >>>>>>>>>>> style in order to set-up easily enforceable rules, I vote with > >>>> both > >>>>>>>> hands > >>>>>>>>>>> for that. > >>>>>>>>>>> > >>>>>>>>>>>> On Fri, Jul 31, 2015 at 3:25 AM, Michael Park < > >> mcyp...@gmail.com > >>>>> > >>>>>>>> wrote: > >>>>>>>>>>>> > >>>>>>>>>>>> Oops, sorry I was so excited that this could just solve the > >> issue > >>>>>>>> that I > >>>>>>>>>>>> forgot to answer your question. > >>>>>>>>>>>> > >>>>>>>>>>>> In general, the clang-format strives to adopt widely used > >> styles, > >>>>>>>> which > >>>>>>>>>> I'm > >>>>>>>>>>>> not sure if we would be considered widely used. Aside from > >> that, > >>>>>>>> another > >>>>>>>>>>>> concern was that it could take a while for our style > >> proposals to > >>>>>>> make > >>>>>>>>>> it > >>>>>>>>>>>> upstream and for it to be useful. > >>>>>>>>>>>> > >>>>>>>>>>>>> On Thu, Jul 30, 2015, 6:12 PM Michael Park < > >> mcyp...@gmail.com> > >>>>>>>> wrote: > >>>>>>>>>>>>> > >>>>>>>>>>>>> Is it worth adding our own style? > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> I noticed other have (LLVM, Google, Chromium, Mozilla, > >> WebKit.). > >>>>>>> How > >>>>>>>>>>>>>> hard is it? > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> I was just looking into this again and *Mozilla* was added as > >>>> the > >>>>>>>>>> newest > >>>>>>>>>>>>> *BreakBeforeBraces* style. It breaks before braces on enum, > >>>>>>> function, > >>>>>>>>>> and > >>>>>>>>>>>>> record definitions (struct, class, union). I think we can > >>>> actually > >>>>>>>> use > >>>>>>>>>>>> that > >>>>>>>>>>>>> one and be done with it. Having looked through the codebase, > >> we > >>>>>>> wrap > >>>>>>>>>> the > >>>>>>>>>>>>> braces for *enum* for about half of the cases. It would be > >> about > >>>>> 35 > >>>>>>>>>>>>> instances that we have to fix from what I can see in our > >>>> codebase. > >>>>>>>> What > >>>>>>>>>>>> do > >>>>>>>>>>>>> you think? > >>>>>>>>>>>>> > >>>>>>>>>>>>> On Thu, Jul 30, 2015 at 5:14 PM Benjamin Mahler < > >>>>>>>>>>>> benjamin.mah...@gmail.com> > >>>>>>>>>>>>> wrote: > >>>>>>>>>>>>> > >>>>>>>>>>>>>> Is it worth adding our own style? > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> I noticed other have (LLVM, Google, Chromium, Mozilla, > >>>> WebKit.). > >>>>>>> How > >>>>>>>>>>>> hard > >>>>>>>>>>>>>> is it? > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> On Thu, Jul 30, 2015 at 4:23 PM, Michael Park < > >>>> mcyp...@gmail.com > >>>>>> > >>>>>>>>>>>> wrote: > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> There are a few syntactical Mesos style guidelines that I > >>>> would > >>>>>>>> like > >>>>>>>>>>>> to > >>>>>>>>>>>>>>> propose to drop. They are: > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> 1. Open braces for namespace should not be wrapped. > >>>>>>>>>>>>>>> *NOTE*: The Google style guide does not wrap the brace > >> after > >>>>>>>>>>>>>>> *namespace*, > >>>>>>>>>>>>>>> and the Mesos style guide does not mention a rule at all. > >> But > >>>> it > >>>>>>>> is > >>>>>>>>>>>>>>> consistent throughout the codebase. > >>>>>>>>>>>>>>> 2. Overloaded operators should be padded with spaces. > >>>>>>>>>>>>>>> 3. Comments should be wrapped at 70 characters. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> The main motivation is that as a community we would like to > >>>>>>> reduce > >>>>>>>>>> the > >>>>>>>>>>>>>>> discrepancy between what *clang-format* produces. This is a > >>>> dual > >>>>>>>>>>>>>> effort, as > >>>>>>>>>>>>>>> we work on improving *clang-format* to support some of our > >>>> style > >>>>>>>>>> which > >>>>>>>>>>>>>> is > >>>>>>>>>>>>>>> popular in the C++ community as well. Wrapping the function > >>>>>>>> arguments > >>>>>>>>>>>> to > >>>>>>>>>>>>>>> avoid "jaggedness" for example is a feature request which > >> is > >>>>>>> being > >>>>>>>>>>>>>> tracked > >>>>>>>>>>>>>>> at: https://llvm.org/bugs/show_bug.cgi?id=23422 > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Going forward, the proposal is to update all of the > >> instances > >>>> of > >>>>>>>> (1) > >>>>>>>>>>>> and > >>>>>>>>>>>>>>> (2) at once. For (3), we can simply relax the constraint > >> from > >>>> 70 > >>>>>>> to > >>>>>>>>>> 80 > >>>>>>>>>>>>>>> without touching the existing comments. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Does anyone have any strong opinions about dropping any of > >>>> the 3 > >>>>>>>>>> rules > >>>>>>>>>>>>>>> above? > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Thanks, > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> MPark. > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>> > >>>>> > >>>> > >> > > > > > > > > -- > > Deshi Xiao > > Twitter: xds2000 > > E-mail: xiaods(AT)gmail.com > >
