Return-Path: X-Original-To: apmail-flink-dev-archive@www.apache.org Delivered-To: apmail-flink-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id CB05917284 for ; Wed, 18 Mar 2015 15:57:20 +0000 (UTC) Received: (qmail 34601 invoked by uid 500); 18 Mar 2015 15:57:15 -0000 Delivered-To: apmail-flink-dev-archive@flink.apache.org Received: (qmail 34538 invoked by uid 500); 18 Mar 2015 15:57:15 -0000 Mailing-List: contact dev-help@flink.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@flink.apache.org Delivered-To: mailing list dev@flink.apache.org Received: (qmail 34527 invoked by uid 99); 18 Mar 2015 15:57:15 -0000 Received: from mail-relay.apache.org (HELO mail-relay.apache.org) (140.211.11.15) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 18 Mar 2015 15:57:15 +0000 Received: from mail-wi0-f179.google.com (mail-wi0-f179.google.com [209.85.212.179]) by mail-relay.apache.org (ASF Mail Server at mail-relay.apache.org) with ESMTPSA id 0B5141A0026 for ; Wed, 18 Mar 2015 15:57:14 +0000 (UTC) Received: by wibg7 with SMTP id g7so66247470wib.1 for ; Wed, 18 Mar 2015 08:57:13 -0700 (PDT) X-Received: by 10.194.77.99 with SMTP id r3mr145431945wjw.149.1426694233608; Wed, 18 Mar 2015 08:57:13 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.155.149 with HTTP; Wed, 18 Mar 2015 08:56:53 -0700 (PDT) In-Reply-To: References: From: Robert Metzger Date: Wed, 18 Mar 2015 16:56:53 +0100 Message-ID: Subject: Re: [DISCUSS] Issues with heterogeneity of the code To: dev@flink.apache.org Content-Type: multipart/alternative; boundary=047d7bfd0918ff5c3b05119222d7 --047d7bfd0918ff5c3b05119222d7 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable I'm against changing the indentation, for the same reasons as Stephan listed. In my opinion, the codebase has grown too large to "just" switch the indentation or the entire code style (to the google style or whatever). We have 235870 LOC of Java and 24173 LOC of Scala. Therefore, I'm proposing to: a) Decide on one validation framework (Guava seems to be our favorite here), change & document it. b) Decide on one parsing framework, change & document it. c) Decide on a list of checkstyle rules. d) Add pull requests for each rule, adding it to the checkstyle configuration and making the code compliant. Regarding the rules themselves: I have the impression that there is an unwritten agreement on the code style by the core committers of Flink. There might be some minor differences nobody cares about .. but all in all the majority of the code is pretty uniform (at least how I perceive it) The list of checkstyle rules should basically enforce this "core committers code style" because it would minimize the number of required changes. Also, the style has evolved over many years and seems to work well for our needs. On Wed, Mar 18, 2015 at 4:25 PM, Stephan Ewen wrote: > I agree, if we set p a new project, we should use space indentation. > > Should we really refactor 300k lines of code? Would be massive. > > Also: The history would basically show a single committer for all code. G= it > blame (for error tracing) would become useless. > > On Wed, Mar 18, 2015 at 3:49 PM, Alexander Alexandrov < > alexander.s.alexandrov@gmail.com> wrote: > > > Massive +1 for switching to space indention. Makes the code render > > consistently across various viewers (e.g. Github UI, Apache > infrastructure, > > IDEs). > > > > 2015-03-18 1:29 GMT+01:00 Fabian Hueske : > > > > > Touching every file of the code would also be a good opportunity to > > switch > > > from tab to space indention. > > > So if we enforce a strict style, we could also address this issue whi= ch > > > causes discussions every now and then. > > > > > > 2015-03-16 21:53 GMT+01:00 Aljoscha Krettek : > > > > > > > No, but I don't know whether that's possible. > > > > > > > > The style guide prescribes, for example, this: > > > > > > > > def foo( > > > > a: Int, > > > > b: String, > > > > c: String) > > > > > > > > for methods with long parameter lists while a lot of people do this= : > > > > > > > > def foo(a: Int, > > > > b: String, > > > > c: String) > > > > > > > > (IntelliJ also does this). > > > > > > > > The scalastyle rules I added supposedly check for the official scal= a > > > > guide style but they allow both styles of methods. > > > > > > > > On Mon, Mar 16, 2015 at 5:37 PM, Till Rohrmann > > > > > wrote: > > > > > Do we already enforce the official Scala style guide strictly? > > > > > > > > > > On Mon, Mar 16, 2015 at 4:57 PM, Aljoscha Krettek < > > aljoscha@apache.org > > > > > > > > > wrote: > > > > > > > > > >> I'm already always sticking to the official Scala style guide, > with > > > the > > > > >> exception of 100 line length. > > > > >> On Mar 16, 2015 3:27 PM, "Till Rohrmann" > > > wrote: > > > > >> > > > > >> > +1 for stricter Java code styles. I haven't looked into the > Google > > > > Code > > > > >> > Style but maybe we make it easier for new contributors if we > > apply a > > > > >> coding > > > > >> > style which is somehow known. > > > > >> > > > > > >> > +1 for line length of 100 for Scala code. I think it makes cod= e > > > > review on > > > > >> > GitHub easier. > > > > >> > > > > > >> > For the Scala style, we could stick to official style guidelin= es > > > [1]. > > > > >> > > > > > >> > [1] http://docs.scala-lang.org/style/ > > > > >> > > > > > >> > On Mon, Mar 16, 2015 at 3:06 PM, Hermann G=C3=A1bor < > > > reckoner42@gmail.com> > > > > >> > wrote: > > > > >> > > > > > >> > > +1 for the stricter Java code styles. > > > > >> > > > > > > >> > > We should not forget about providing code formatter settings > for > > > > >> Eclipse > > > > >> > > and Intellij IDEA (as mentioned above). > > > > >> > > That would help a lot. > > > > >> > > > > > > >> > > (Of course if we'll use Google Code Style, they already > provide > > > such > > > > >> > files > > > > >> > > < > > > > >> > > > > > > >> > > > > > >> > > > > > > > > > > https://code.google.com/p/google-styleguide/source/browse/trunk/intellij-= java-google-style.xml > > > > >> > > > > > > > >> > > .) > > > > >> > > > > > > >> > > On Mon, Mar 16, 2015 at 2:45 PM Alexander Alexandrov < > > > > >> > > alexander.s.alexandrov@gmail.com> wrote: > > > > >> > > > > > > >> > > > +1 for not limiting the line length. > > > > >> > > > > > > > >> > > > 2015-03-16 14:39 GMT+01:00 Stephan Ewen = : > > > > >> > > > > > > > >> > > > > +1 for not limiting the line length. Everyone should hav= e > a > > > good > > > > >> > sense > > > > >> > > to > > > > >> > > > > break lines. When in exceptional cases people violate > this, > > it > > > > is > > > > >> > > usually > > > > >> > > > > for a good reason. > > > > >> > > > > > > > > >> > > > > On Mon, Mar 16, 2015 at 2:18 PM, Maximilian Michels < > > > > >> mxm@apache.org> > > > > >> > > > > wrote: > > > > >> > > > > > > > > >> > > > > > +1 for enforcing a more strict Java code style. Howeve= r, > > > let's > > > > >> not > > > > >> > > > > > introduce a line legth of 100 like in Scala. I think > > that's > > > > >> hurting > > > > >> > > > > > readability of the code. > > > > >> > > > > > > > > > >> > > > > > On Sat, Mar 14, 2015 at 4:41 PM, Ufuk Celebi < > > > uce@apache.org> > > > > >> > wrote: > > > > >> > > > > > > > > > >> > > > > > > On Saturday, March 14, 2015, Aljoscha Krettek < > > > > >> > aljoscha@apache.org > > > > >> > > > > > > > >> > > > > > wrote: > > > > >> > > > > > > > > > > >> > > > > > > > I'm in favor of strict coding styles. And I like t= he > > > > google > > > > >> > > style. > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > +1 I would like that. We essentially all agree that = we > > > want > > > > >> more > > > > >> > > > > > > homogeneity and I think strict rules are the only wa= y > to > > > go. > > > > >> > Since > > > > >> > > > this > > > > >> > > > > > is > > > > >> > > > > > > a very subjective matter it makes sense to go with > > > something > > > > >> > > > (somewhat) > > > > >> > > > > > > well > > > > >> > > > > > > established like the Google code style. > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > > > > > > > --047d7bfd0918ff5c3b05119222d7--