OK, I'll try to summarize the discussion so far (please correct me if I got something wrong):
Everybody is in favor of adding a stricter code style based on the Google Java code style. Main points of discussion are: 1) Line length 2) JavaDocs 3) Tabs vs. Spaces -- Line length Issue: Google code style demands 80 or 100 chars line length limit. Some people find this too narrow. Discussion so far: Nobody objected against a limit of 120 chars, AFAIR. -- JavaDocs Issue: Google code style says "At the *minimum*, Javadoc is present for every public class, and every public or protected member of such a class, with a few exceptions noted below." Exceptions are self-explanatory methods like getters and setters and overwritten method. Significant parts of the Flink code base do not comply with this requirement. It would be a huge effort to add the corresponding code documentation. Discussion so far: Disable JavaDoc checks when adding the code style and opening JIRAs for adding JavaDocs on a per Maven module level (with potentially subissues) with target dates. Once a module complies to the rule, the JavaDoc check is enabled in the checkstyle. -- Tabs vs. Spaces Issue: The Google code style requires two spaces for indention. The Java code base of Flink uses tabs. Moving from tabs to spaces would touch every line of Java code and therefore might make the tracking of changes in the past much harder. Max provided some numbers for applying the Google code style on the current code base: The style checker found 28k violations (possibly multiple ones per LOC) on 121k LOC of Java code in 1601 out of 3251 Java classes. So Google code style without spaces would touch every second file and about every 4th line of code. The question I have, would it be easier to track history with a commit that touches 1/2 (or 1/4) of the code base compared to one that touches everything. Discussion so far: AFAIR, nobody said to have a personal preferences of tabs over spaces. However, some people raised concerns that the implied changes of moving the code base from tabs to spaces would be too massive and the "benefits" would not justify the "cost" of the change. I think, this is the main point of discussion, right now. If we want to come to a conclusion, we should not let this discussion fall asleep (as happened a few times in the past). Does anybody NOT agree with the "solutions" for line length and JavaDocs, i.e., 120 chars and "lazy" adding of JavaDocs with JIRA tracking, or has another proposal? How can we resolve the Tabs vs. Spaces discussion? Cheers, Fabian 2015-10-30 18:16 GMT+01:00 Maximilian Michels <m...@apache.org>: > I looked up if the Checkstyle plugin would also support tabs with a > fixed line length. Indeed, this is possible because a tab can be > mapped to a fixed number of spaces. > > I've modified the default Google Style Checkstyle file. I changed the > indention to tabs (2 spaces) and increased the line length to 120: > https://gist.github.com/mxm/2ca4ef7702667c167d10 > > The scan of the entire Flink project resulted in 27,992 items in 1601 > files. This is roughly corresponds to the number of lines we would > have to touch to comply with the style rules. Note that, one line may > contain multiple items. A lot of the items are import statements. > > Next, I tried running the vanilla Google Style Checkstyle file over > the entire code base but my IntelliJ crashed. Using Maven, I wasn't > able to get a total result displayed but I'm assuming it would be > almost all lines of Flink code that had a violation due to tabs. > > On Mon, Oct 26, 2015 at 6:56 PM, Suneel Marthi <suneel.mar...@gmail.com> > wrote: > > 2 spaces is the convention that's followed on Mahout and Oryx. > > > > On Mon, Oct 26, 2015 at 1:42 PM, Till Rohrmann <trohrm...@apache.org> > wrote: > > > >> Concerning question 2 Tabs vs. Spaces, in case of spaces we would have > to > >> decide on the number of spaces, too. The Google Java style says to use > a 2 > >> space indentation, which is in my opinion sufficient to distinguish > >> different indentations levels from each other. Furthermore, it would > save > >> some space. > >> > >> I would not vote -1 if we keep tabs. > >> > >> > >> > >> On Sat, Oct 24, 2015 at 8:33 PM, Henry Saputra <henry.sapu...@gmail.com > > > >> wrote: > >> > >> > +1 for adding restriction for Javadoc at least at the header of public > >> > classes and methods. > >> > > >> > We did the exercise in Twill and seemed to work pretty well. > >> > > >> > On Fri, Oct 23, 2015 at 1:34 AM, Maximilian Michels <m...@apache.org> > >> > wrote: > >> > > I don't think lazily adding comments will work. However, I'm fine > with > >> > > adding all the checkstyle rules one module at a time (with a jira > >> > > issue to keep track of the modules already converted). It's not > going > >> > > to happen that we lazily add comments because that's the reason why > >> > > comments are missing in the first place... > >> > > > >> > > On Fri, Oct 23, 2015 at 12:05 AM, Henry Saputra < > >> henry.sapu...@gmail.com> > >> > wrote: > >> > >> Could we make certain rules to give warning instead of error? > >> > >> > >> > >> This would allow us to cherry-pick certain rules we would like > people > >> > >> to follow but not strictly enforced. > >> > >> > >> > >> - Henry > >> > >> > >> > >> On Thu, Oct 22, 2015 at 9:20 AM, Stephan Ewen <se...@apache.org> > >> wrote: > >> > >>> I don't think a "let add comments to everything" effort gives us > good > >> > >>> comments, actually. It just gives us checkmark comments that make > the > >> > rules > >> > >>> pass. > >> > >>> > >> > >>> On Thu, Oct 22, 2015 at 3:29 PM, Fabian Hueske <fhue...@gmail.com > > > >> > wrote: > >> > >>> > >> > >>>> Sure, I don't expect it to be free. > >> > >>>> But everybody should be aware of the cost of adding this code > style, > >> > i.e., > >> > >>>> spending a huge amount of time on reformatting and documenting > code. > >> > >>>> > >> > >>>> Alternatively, we could drop the JavaDocs rule and make the > >> transition > >> > >>>> significantly cheaper. > >> > >>>> > >> > >>>> 2015-10-22 15:24 GMT+02:00 Till Rohrmann <trohrm...@apache.org>: > >> > >>>> > >> > >>>> > There ain’t no such thing as a free lunch and code style. > >> > >>>> > > >> > >>>> > On Thu, Oct 22, 2015 at 3:13 PM, Maximilian Michels < > >> m...@apache.org > >> > > > >> > >>>> > wrote: > >> > >>>> > > >> > >>>> > > I think we have to document all these classes. Code Style > >> doesn't > >> > come > >> > >>>> > > for free :) > >> > >>>> > > > >> > >>>> > > On Thu, Oct 22, 2015 at 3:09 PM, Fabian Hueske < > >> fhue...@gmail.com > >> > > > >> > >>>> > wrote: > >> > >>>> > > > Any ideas how to deal with the mandatory JavaDoc rule for > >> > existing > >> > >>>> > code? > >> > >>>> > > > Just adding empty headers to make the checkstyle pass or > >> start a > >> > >>>> > serious > >> > >>>> > > > effort to add the missing docs? > >> > >>>> > > > > >> > >>>> > > > 2015-10-21 13:31 GMT+02:00 Matthias J. Sax < > mj...@apache.org > >> >: > >> > >>>> > > > > >> > >>>> > > >> Agreed. That's the reason why I am in favor of using > vanilla > >> > Google > >> > >>>> > code > >> > >>>> > > >> style. > >> > >>>> > > >> > >> > >>>> > > >> On 10/21/2015 12:31 PM, Stephan Ewen wrote: > >> > >>>> > > >> > We started out originally with mixed tab/spaces, but it > >> > ended up > >> > >>>> > with > >> > >>>> > > >> > people mixing spaces and tabs arbitrarily, and there is > >> > little way > >> > >>>> > to > >> > >>>> > > >> > enforce Matthias' specific suggestion via checkstyle. > >> > >>>> > > >> > That's why we dropped spaces alltogether... > >> > >>>> > > >> > > >> > >>>> > > >> > On Wed, Oct 21, 2015 at 12:03 PM, Gyula Fóra < > >> > >>>> gyula.f...@gmail.com> > >> > >>>> > > >> wrote: > >> > >>>> > > >> > > >> > >>>> > > >> >> I think the nice thing about a common codestyle is that > >> > everyone > >> > >>>> > can > >> > >>>> > > set > >> > >>>> > > >> >> the template in the IDE and use the formatting > commands. > >> > >>>> > > >> >> > >> > >>>> > > >> >> Matthias's suggestion makes this practically > impossible so > >> > -1 for > >> > >>>> > > mixed > >> > >>>> > > >> >> tabs/spaces from my side. > >> > >>>> > > >> >> > >> > >>>> > > >> >> Matthias J. Sax <mj...@apache.org> ezt írta (időpont: > >> > 2015. okt. > >> > >>>> > > 21., > >> > >>>> > > >> Sze, > >> > >>>> > > >> >> 11:46): > >> > >>>> > > >> >> > >> > >>>> > > >> >>> I actually like tabs a lot, however, in a "mixed" > style > >> > together > >> > >>>> > > with > >> > >>>> > > >> >>> spaces. Example: > >> > >>>> > > >> >>> > >> > >>>> > > >> >>> myVar.callMethod(param1, // many more > >> > >>>> > > >> >>> .................paramX); // the dots mark > space > >> > >>>> indention > >> > >>>> > > >> >>> > >> > >>>> > > >> >>> indenting "paramX" with tabs does not give nice > aliment. > >> > Not > >> > >>>> sure > >> > >>>> > if > >> > >>>> > > >> >>> this would be a feasible compromise to keeps tabs in > >> > general, > >> > >>>> but > >> > >>>> > > use > >> > >>>> > > >> >>> space for cases as above. > >> > >>>> > > >> >>> > >> > >>>> > > >> >>> If this in no feasible compromise, I would prefer > space > >> to > >> > get > >> > >>>> the > >> > >>>> > > >> >>> correct indention in examples as above. Even if this > >> > result in a > >> > >>>> > > >> >>> complete reformatting of the whole code. > >> > >>>> > > >> >>> > >> > >>>> > > >> >>> > >> > >>>> > > >> >>> Why this? Everybody can set this in it's IDE/editor as > >> > he/she > >> > >>>> > > wishes... > >> > >>>> > > >> >>> > >> > >>>> > > >> >>>>> If we keep tabs, we will have to specify the line > >> length > >> > >>>> > relative > >> > >>>> > > to > >> > >>>> > > >> a > >> > >>>> > > >> >>> tab > >> > >>>> > > >> >>>>> size (like 4). > >> > >>>> > > >> >>> > >> > >>>> > > >> >>> > >> > >>>> > > >> >>> -Matthias > >> > >>>> > > >> >>> > >> > >>>> > > >> >>> > >> > >>>> > > >> >>> > >> > >>>> > > >> >>> > >> > >>>> > > >> >>> > >> > >>>> > > >> >>> On 10/21/2015 11:06 AM, Ufuk Celebi wrote: > >> > >>>> > > >> >>>> To summarize up to this point: > >> > >>>> > > >> >>>> > >> > >>>> > > >> >>>> - All are in favour of Google check style (with the > >> > following > >> > >>>> > > possible > >> > >>>> > > >> >>>> exceptions) > >> > >>>> > > >> >>>> - Proposed exceptions so far: > >> > >>>> > > >> >>>> * Specific line length 100 vs. 120 characters > >> > >>>> > > >> >>>> * Keep tabs instead converting to spaces (this > would > >> > >>>> translate > >> > >>>> > to > >> > >>>> > > >> >>>> skipping/coming up with some indentation rules as > well) > >> > >>>> > > >> >>>> > >> > >>>> > > >> >>>> If we keep tabs, we will have to specify the line > length > >> > >>>> relative > >> > >>>> > > to a > >> > >>>> > > >> >>> tab > >> > >>>> > > >> >>>> size (like 4). > >> > >>>> > > >> >>>> > >> > >>>> > > >> >>>> Let’s keep the discussion going a little longer. I > think > >> > it has > >> > >>>> > > >> >> proceeded > >> > >>>> > > >> >>>> in a very reasonable manner so far. Thanks for this! > >> > >>>> > > >> >>>> > >> > >>>> > > >> >>>> – Ufuk > >> > >>>> > > >> >>>> > >> > >>>> > > >> >>>> On Wed, Oct 21, 2015 at 10:29 AM, Fabian Hueske < > >> > >>>> > fhue...@gmail.com > >> > >>>> > > > > >> > >>>> > > >> >>> wrote: > >> > >>>> > > >> >>>> > >> > >>>> > > >> >>>>> Thanks Max for checking the modifications by the > Google > >> > code > >> > >>>> > > style. > >> > >>>> > > >> >>>>> It is very good to know, that the impact on the code > >> base > >> > >>>> would > >> > >>>> > > not > >> > >>>> > > >> be > >> > >>>> > > >> >>> too > >> > >>>> > > >> >>>>> massive. If the Google code style would have touched > >> > almost > >> > >>>> > every > >> > >>>> > > >> >> line, > >> > >>>> > > >> >>> I > >> > >>>> > > >> >>>>> would have been in favor of converting to spaces. > >> > However, > >> > >>>> your > >> > >>>> > > >> >>> assessment > >> > >>>> > > >> >>>>> is a strong argument to continue with tabs, IMO. > >> > >>>> > > >> >>>>> > >> > >>>> > > >> >>>>> Regarding the line length limit, I personally find > 100 > >> > chars > >> > >>>> too > >> > >>>> > > >> >> narrow > >> > >>>> > > >> >>> but > >> > >>>> > > >> >>>>> would be +1 for having a limit. > >> > >>>> > > >> >>>>> > >> > >>>> > > >> >>>>> +1 for discussing the Scala style in a separate > thread. > >> > >>>> > > >> >>>>> > >> > >>>> > > >> >>>>> Fabian > >> > >>>> > > >> >>>>> > >> > >>>> > > >> >>>>> 2015-10-20 18:12 GMT+02:00 Maximilian Michels < > >> > m...@apache.org > >> > >>>> >: > >> > >>>> > > >> >>>>> > >> > >>>> > > >> >>>>>> I'm a little less excited about this. You might > not be > >> > aware > >> > >>>> > but, > >> > >>>> > > >> for > >> > >>>> > > >> >>>>>> a large portion of the source code, we already > follow > >> > the > >> > >>>> > Google > >> > >>>> > > >> >> style > >> > >>>> > > >> >>>>>> guide. The main changes will be tabs->spaces and > >> 80/100 > >> > >>>> > > characters > >> > >>>> > > >> >>>>>> line limit. > >> > >>>> > > >> >>>>>> > >> > >>>> > > >> >>>>>> Out of curiosity, I ran the official Google Style > >> > Checkstyle > >> > >>>> > > >> >>>>>> configuration to confirm my suspicion: > >> > >>>> > > >> >>>>>> > >> > >>>> > > >> >>>>>> > >> > >>>> > > >> >>>>> > >> > >>>> > > >> >>> > >> > >>>> > > >> >> > >> > >>>> > > >> > >> > >>>> > > > >> > >>>> > > >> > >>>> > >> > > >> > https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/google_checks.xml > >> > >>>> > > >> >>>>>> The changes are very little if we turn off line > length > >> > limit > >> > >>>> > and > >> > >>>> > > >> >>>>>> tabs-to-spaces conversion. > >> > >>>> > > >> >>>>>> > >> > >>>> > > >> >>>>>> There are some things I really like about the > Google > >> > style, > >> > >>>> > e.g. > >> > >>>> > > >> >> every > >> > >>>> > > >> >>>>>> class has to have a JavaDoc and spaces after > keywords > >> > (can't > >> > >>>> > > stand > >> > >>>> > > >> if > >> > >>>> > > >> >>>>>> there aren't any). I'm not sure if we should change > >> > tabs to > >> > >>>> > > spaces, > >> > >>>> > > >> >>>>>> because it means touching almost every single line > of > >> > code. > >> > >>>> > > However, > >> > >>>> > > >> >>>>>> if we keep the tabs, we cannot make use of the > >> different > >> > >>>> > > indention > >> > >>>> > > >> >> for > >> > >>>> > > >> >>>>>> case statements or wrapped lines...maybe that's a > >> > compromise > >> > >>>> we > >> > >>>> > > can > >> > >>>> > > >> >>>>>> live with. > >> > >>>> > > >> >>>>>> > >> > >>>> > > >> >>>>>> If we introduce the Google Style for Java, will we > >> also > >> > >>>> impose > >> > >>>> > a > >> > >>>> > > >> >>>>>> stricter style check for Scala? IMHO the line > length > >> is > >> > the > >> > >>>> > > >> strictest > >> > >>>> > > >> >>>>>> part of the Scala Checkstyle. > >> > >>>> > > >> >>>>>> > >> > >>>> > > >> >>>>>> > >> > >>>> > > >> >>>>>> On Tue, Oct 20, 2015 at 4:14 PM, Henry Saputra < > >> > >>>> > > >> >>> henry.sapu...@gmail.com> > >> > >>>> > > >> >>>>>> wrote: > >> > >>>> > > >> >>>>>>> 1) yes. Been dancing this issue for a while. Let's > >> > pull the > >> > >>>> > > >> trigger. > >> > >>>> > > >> >>>>> Did > >> > >>>> > > >> >>>>>>> the exercise with Tachyon while back and did help > >> > >>>> readability > >> > >>>> > > and > >> > >>>> > > >> >>>>>>> homogeneity of code. > >> > >>>> > > >> >>>>>>> > >> > >>>> > > >> >>>>>>> 2) +1 for Google Java style with documented > >> exceptions > >> > and > >> > >>>> > > >> >> explanation > >> > >>>> > > >> >>>>> on > >> > >>>> > > >> >>>>>>> why. > >> > >>>> > > >> >>>>>>> > >> > >>>> > > >> >>>>>>> On Tuesday, October 20, 2015, Ufuk Celebi < > >> > u...@apache.org> > >> > >>>> > > wrote: > >> > >>>> > > >> >>>>>>> > >> > >>>> > > >> >>>>>>>> DISCLAIMER: This is not my personal idea, but a > >> > community > >> > >>>> > > >> >> discussion > >> > >>>> > > >> >>>>>> from > >> > >>>> > > >> >>>>>>>> some time ago. Don't kill the messenger. > >> > >>>> > > >> >>>>>>>> > >> > >>>> > > >> >>>>>>>> In March we were discussing issues with > >> heterogeneity > >> > of > >> > >>>> the > >> > >>>> > > code > >> > >>>> > > >> >>> [1]. > >> > >>>> > > >> >>>>>> The > >> > >>>> > > >> >>>>>>>> summary is that we had a consensus to enforce a > >> > stricter > >> > >>>> code > >> > >>>> > > >> style > >> > >>>> > > >> >>> on > >> > >>>> > > >> >>>>>> our > >> > >>>> > > >> >>>>>>>> Java code base in order to make it easier to > switch > >> > between > >> > >>>> > > >> >> projects > >> > >>>> > > >> >>>>>> and to > >> > >>>> > > >> >>>>>>>> have clear rules for new contributions. The main > >> > proposal > >> > >>>> in > >> > >>>> > > the > >> > >>>> > > >> >> last > >> > >>>> > > >> >>>>>>>> discussion was to go with Google's Java code > style. > >> > Not all > >> > >>>> > > were > >> > >>>> > > >> >>> fully > >> > >>>> > > >> >>>>>>>> satisfied with this, but still everyone agreed on > >> > some kind > >> > >>>> > of > >> > >>>> > > >> >> style. > >> > >>>> > > >> >>>>>>>> > >> > >>>> > > >> >>>>>>>> I think the upcoming 0.10 release is a good > point to > >> > >>>> finally > >> > >>>> > go > >> > >>>> > > >> >>>>> through > >> > >>>> > > >> >>>>>>>> with these changes (right after the > >> > release/branch-off). > >> > >>>> > > >> >>>>>>>> > >> > >>>> > > >> >>>>>>>> I propose to go with Google's Java code style > [2] as > >> > >>>> proposed > >> > >>>> > > >> >>> earlier. > >> > >>>> > > >> >>>>>>>> > >> > >>>> > > >> >>>>>>>> PROs: > >> > >>>> > > >> >>>>>>>> - Clear style guide available > >> > >>>> > > >> >>>>>>>> - Tooling like checkstyle rules, IDE plugins > already > >> > >>>> > available > >> > >>>> > > >> >>>>>>>> > >> > >>>> > > >> >>>>>>>> CONs: > >> > >>>> > > >> >>>>>>>> - Fully breaks our current style > >> > >>>> > > >> >>>>>>>> > >> > >>>> > > >> >>>>>>>> The main problem with this will be open pull > >> requests, > >> > >>>> which > >> > >>>> > > will > >> > >>>> > > >> >> be > >> > >>>> > > >> >>>>>> harder > >> > >>>> > > >> >>>>>>>> to merge after all the changes. On the other > hand, > >> > should > >> > >>>> > pull > >> > >>>> > > >> >>>>> requests > >> > >>>> > > >> >>>>>>>> that have been open for a long time block this? > Most > >> > of the > >> > >>>> > > >> >> important > >> > >>>> > > >> >>>>>>>> changes will be merged for the release anyways. I > >> > think in > >> > >>>> > the > >> > >>>> > > >> long > >> > >>>> > > >> >>>>> run > >> > >>>> > > >> >>>>>> we > >> > >>>> > > >> >>>>>>>> will gain more than we loose by this (more > >> homogenous > >> > code, > >> > >>>> > > clear > >> > >>>> > > >> >>>>>> rules). > >> > >>>> > > >> >>>>>>>> And it is questionable whether we will ever be > able > >> > to do > >> > >>>> > such > >> > >>>> > > a > >> > >>>> > > >> >>>>> change > >> > >>>> > > >> >>>>>> in > >> > >>>> > > >> >>>>>>>> the future if we cannot do it now. The project > will > >> > most > >> > >>>> > likely > >> > >>>> > > >> >> grow > >> > >>>> > > >> >>>>> and > >> > >>>> > > >> >>>>>>>> attract more contributors, at which point it > will be > >> > even > >> > >>>> > > harder > >> > >>>> > > >> to > >> > >>>> > > >> >>>>> do. > >> > >>>> > > >> >>>>>>>> > >> > >>>> > > >> >>>>>>>> Please make sure to answer the following points > in > >> the > >> > >>>> > > discussion: > >> > >>>> > > >> >>>>>>>> > >> > >>>> > > >> >>>>>>>> 1) Are you (still) in favour of enforcing > stricter > >> > rules on > >> > >>>> > the > >> > >>>> > > >> >> Java > >> > >>>> > > >> >>>>>>>> codebase? > >> > >>>> > > >> >>>>>>>> > >> > >>>> > > >> >>>>>>>> 2) If yes, would you be OK with the Google's Java > >> code > >> > >>>> style? > >> > >>>> > > >> >>>>>>>> > >> > >>>> > > >> >>>>>>>> – Ufuk > >> > >>>> > > >> >>>>>>>> > >> > >>>> > > >> >>>>>>>> [1] > >> > >>>> > > >> >>>>>>>> > >> > >>>> > > >> >>>>>>>> > >> > >>>> > > >> >>>>>> > >> > >>>> > > >> >>>>> > >> > >>>> > > >> >>> > >> > >>>> > > >> >> > >> > >>>> > > >> > >> > >>>> > > > >> > >>>> > > >> > >>>> > >> > > >> > http://mail-archives.apache.org/mod_mbox/flink-dev/201503.mbox/%3ccanc1h_von0b5omnwzxchtyzwhakeghbzvquyk7s9o2a36b8...@mail.gmail.com%3e > >> > >>>> > > >> >>>>>>>> > >> > >>>> > > >> >>>>>>>> [2] > >> > https://google.github.io/styleguide/javaguide.html > >> > >>>> > > >> >>>>>>>> > >> > >>>> > > >> >>>>>> > >> > >>>> > > >> >>>>> > >> > >>>> > > >> >>>> > >> > >>>> > > >> >>> > >> > >>>> > > >> >>> > >> > >>>> > > >> >> > >> > >>>> > > >> > > >> > >>>> > > >> > >> > >>>> > > >> > >> > >>>> > > > >> > >>>> > > >> > >>>> > >> > > >> >
