Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.0
    • Component/s: DataStream API
    • Labels:
      None

      Description

      There was some consensus on the ML (https://lists.apache.org/thread.html/94c8c5186b315c58c3f8aaf536501b99e8b92ee97b0034dee295ff6a@%3Cdev.flink.apache.org%3E) that we want to have a more uniform code style. We should start module-by-module and by introducing increasingly stricter rules. We have to be aware of the PR situation and ensure that we have minimal breakage for contributors.

      This issue aims at adding a custom checkstyle.xml for flink-streaming-java that is based on our current checkstyle.xml but adds these checks for Javadocs:

      <!--
      
      JAVADOC CHECKS
      
      -->
      
      <!-- Checks for Javadoc comments.                     -->
      <!-- See http://checkstyle.sf.net/config_javadoc.html -->
      <module name="JavadocMethod">
        <property name="scope" value="protected"/>
        <property name="severity" value="error"/>
        <property name="allowMissingJavadoc" value="true"/>
        <property name="allowMissingParamTags" value="true"/>
        <property name="allowMissingReturnTag" value="true"/>
        <property name="allowMissingThrowsTags" value="true"/>
        <property name="allowThrowsTagsForSubclasses" value="true"/>
        <property name="allowUndeclaredRTE" value="true"/>
      </module>
      
      <!-- Check that paragraph tags are used correctly in Javadoc. -->
      <module name="JavadocParagraph"/>
      
      <module name="JavadocType">
        <property name="scope" value="protected"/>
        <property name="severity" value="error"/>
        <property name="allowMissingParamTags" value="true"/>
      </module>
      
      <module name="JavadocStyle">
        <property name="severity" value="error"/>
        <property name="checkHtml" value="true"/>
      </module>
      

      This checks:

      • Every type has a type-level Javadoc
      • Proper use of <p> in Javadocs
      • First sentence must end with a proper punctuation mark
      • Proper use (including closing) of HTML tags

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user aljoscha opened a pull request:

          https://github.com/apache/flink/pull/3567

          FLINK-6107 Add custom checkstyle for flink-streaming-java

          I played around with this a bit over the weekend... 😃

          This is a proposal for a way forward from the discussion on code style on the mailing list: https://lists.apache.org/thread.html/94c8c5186b315c58c3f8aaf536501b99e8b92ee97b0034dee295ff6a@%3Cdev.flink.apache.org%3E.

          This adds a custom `checkstyle.xml` based on the Apache Beam checkstyle for `flink-streaming-java` only. The code style is not really different form what we currently have, this just aims at unifying the existing codebase. The Flink specific changes are mostly:

          • Tabs instead of spaces
          • No line-length limit
          • We disallow a bunch of imports

          The first commit adds the checkstyle file but a lot of stuff is commented out because the Flink code base would fail those tests. The following commits subsequently add more and more invasive changes along with enabling more rules.

          The commits up to the last two commits at uncontroversial (in my opinion) checks that will make code reviews a lot easier in the future. Especially having an enforced import order and trailing whitespace policy will make import order changes and whitespace changes a thing of the past. The checks for spacing around unary and binary operators should also be unproblematic.

          The last two commits normalise the style of code blocks. Having a unified style for those should be good in the future and will prevent "edit wars" (same as for import order and whitespace) with people that have their IDE formatter configured differently.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/aljoscha/flink strict-checkstyle-streaming-beam-style

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/3567.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #3567


          commit 78260ad4a349c7dc64d56742085899bc6c633e66
          Author: Aljoscha Krettek <aljoscha.krettek@gmail.com>
          Date: 2017-03-17T14:32:23Z

          FLINK-6107 Custom checkstyle for flink-streaming-java

          This is based on the Apache Beam checkstyle with some custom
          modifications for making it fit the exisiting Flink code base. The most
          notable change is that Flink uses Tabs for indentation while Beam uses
          spaces.

          This adds a lot of checks that are commented out because they require
          non-trivial changes to the code base.

          There are some trivial code changes for good practices checkstyle rules
          that where only broken in very few places.

          commit efbda9e9e6a88340d447988d90e1bd40d0940791
          Author: Aljoscha Krettek <aljoscha.krettek@gmail.com>
          Date: 2017-03-19T09:29:10Z

          FLINK-6107 Enable newline at EOF check in streaming checkstyle

          commit 42306cffcd0082ea5077892fb84e40371ba81377
          Author: Aljoscha Krettek <aljoscha.krettek@gmail.com>
          Date: 2017-03-19T10:17:47Z

          FLINK-6107 Enable trailing whitespace check in streaming checkstyle

          commit e8dd1759d059ac138b7a528ef4775217fbf55742
          Author: Aljoscha Krettek <aljoscha.krettek@gmail.com>
          Date: 2017-03-19T10:21:17Z

          FLINK-6107 Enable Javadoc checks in streaming checkstyle

          commit 4cd343c03fcbef3c8115005346d03a583e467849
          Author: Aljoscha Krettek <aljoscha.krettek@gmail.com>
          Date: 2017-03-19T11:23:22Z

          FLINK-6107 Enable import order check in streaming checkstyle

          commit bbc7d75bc692656707b70b844b044e71259d148a
          Author: Aljoscha Krettek <aljoscha.krettek@gmail.com>
          Date: 2017-03-19T13:54:35Z

          FLINK-6107 Enable WhitespaceAfter check in streaming checkstyle

          commit ccb3e28a66d17daee08bb71c89cdaf82fefc412b
          Author: Aljoscha Krettek <aljoscha.krettek@gmail.com>
          Date: 2017-03-19T14:02:00Z

          FLINK-6107 Enable WhitespaceAround check in streaming checkstyle

          commit 66f8f9b47f646ddaf2a333ec68ba51edac306c16
          Author: Aljoscha Krettek <aljoscha.krettek@gmail.com>
          Date: 2017-03-19T14:18:21Z

          FLINK-6107 Enable RightCurly check in streaming checkstyle

          commit e061edd14db4e3af6844697194aee78324170508
          Author: Aljoscha Krettek <aljoscha.krettek@gmail.com>
          Date: 2017-03-19T14:25:24Z

          FLINK-6107 Enable LeftCurly check in streaming checkstyle

          commit a88fb33dc02781e04030d7c91351659937d3a836
          Author: Aljoscha Krettek <aljoscha.krettek@gmail.com>
          Date: 2017-03-19T14:29:02Z

          FLINK-6107 Enable MemberNameCheck in streaming checkstyle

          commit 181d8b3649293b143106fe66d6a3b5c6b3dbaf93
          Author: Aljoscha Krettek <aljoscha.krettek@gmail.com>
          Date: 2017-03-19T14:30:23Z

          FLINK-6107 Enable StaticVariableNameCheck in streaming checkstyle


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user aljoscha opened a pull request: https://github.com/apache/flink/pull/3567 FLINK-6107 Add custom checkstyle for flink-streaming-java I played around with this a bit over the weekend... 😃 This is a proposal for a way forward from the discussion on code style on the mailing list: https://lists.apache.org/thread.html/94c8c5186b315c58c3f8aaf536501b99e8b92ee97b0034dee295ff6a@%3Cdev.flink.apache.org%3E . This adds a custom `checkstyle.xml` based on the Apache Beam checkstyle for `flink-streaming-java` only. The code style is not really different form what we currently have, this just aims at unifying the existing codebase. The Flink specific changes are mostly: Tabs instead of spaces No line-length limit We disallow a bunch of imports The first commit adds the checkstyle file but a lot of stuff is commented out because the Flink code base would fail those tests. The following commits subsequently add more and more invasive changes along with enabling more rules. The commits up to the last two commits at uncontroversial (in my opinion) checks that will make code reviews a lot easier in the future. Especially having an enforced import order and trailing whitespace policy will make import order changes and whitespace changes a thing of the past. The checks for spacing around unary and binary operators should also be unproblematic. The last two commits normalise the style of code blocks. Having a unified style for those should be good in the future and will prevent "edit wars" (same as for import order and whitespace) with people that have their IDE formatter configured differently. You can merge this pull request into a Git repository by running: $ git pull https://github.com/aljoscha/flink strict-checkstyle-streaming-beam-style Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3567.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3567 commit 78260ad4a349c7dc64d56742085899bc6c633e66 Author: Aljoscha Krettek <aljoscha.krettek@gmail.com> Date: 2017-03-17T14:32:23Z FLINK-6107 Custom checkstyle for flink-streaming-java This is based on the Apache Beam checkstyle with some custom modifications for making it fit the exisiting Flink code base. The most notable change is that Flink uses Tabs for indentation while Beam uses spaces. This adds a lot of checks that are commented out because they require non-trivial changes to the code base. There are some trivial code changes for good practices checkstyle rules that where only broken in very few places. commit efbda9e9e6a88340d447988d90e1bd40d0940791 Author: Aljoscha Krettek <aljoscha.krettek@gmail.com> Date: 2017-03-19T09:29:10Z FLINK-6107 Enable newline at EOF check in streaming checkstyle commit 42306cffcd0082ea5077892fb84e40371ba81377 Author: Aljoscha Krettek <aljoscha.krettek@gmail.com> Date: 2017-03-19T10:17:47Z FLINK-6107 Enable trailing whitespace check in streaming checkstyle commit e8dd1759d059ac138b7a528ef4775217fbf55742 Author: Aljoscha Krettek <aljoscha.krettek@gmail.com> Date: 2017-03-19T10:21:17Z FLINK-6107 Enable Javadoc checks in streaming checkstyle commit 4cd343c03fcbef3c8115005346d03a583e467849 Author: Aljoscha Krettek <aljoscha.krettek@gmail.com> Date: 2017-03-19T11:23:22Z FLINK-6107 Enable import order check in streaming checkstyle commit bbc7d75bc692656707b70b844b044e71259d148a Author: Aljoscha Krettek <aljoscha.krettek@gmail.com> Date: 2017-03-19T13:54:35Z FLINK-6107 Enable WhitespaceAfter check in streaming checkstyle commit ccb3e28a66d17daee08bb71c89cdaf82fefc412b Author: Aljoscha Krettek <aljoscha.krettek@gmail.com> Date: 2017-03-19T14:02:00Z FLINK-6107 Enable WhitespaceAround check in streaming checkstyle commit 66f8f9b47f646ddaf2a333ec68ba51edac306c16 Author: Aljoscha Krettek <aljoscha.krettek@gmail.com> Date: 2017-03-19T14:18:21Z FLINK-6107 Enable RightCurly check in streaming checkstyle commit e061edd14db4e3af6844697194aee78324170508 Author: Aljoscha Krettek <aljoscha.krettek@gmail.com> Date: 2017-03-19T14:25:24Z FLINK-6107 Enable LeftCurly check in streaming checkstyle commit a88fb33dc02781e04030d7c91351659937d3a836 Author: Aljoscha Krettek <aljoscha.krettek@gmail.com> Date: 2017-03-19T14:29:02Z FLINK-6107 Enable MemberNameCheck in streaming checkstyle commit 181d8b3649293b143106fe66d6a3b5c6b3dbaf93 Author: Aljoscha Krettek <aljoscha.krettek@gmail.com> Date: 2017-03-19T14:30:23Z FLINK-6107 Enable StaticVariableNameCheck in streaming checkstyle
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on the issue:

          https://github.com/apache/flink/pull/3567

          Love the checks that were enabled. They really cover most (if not all) issues that we have to raise in reviews at the moment.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/3567 Love the checks that were enabled. They really cover most (if not all) issues that we have to raise in reviews at the moment.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3567#discussion_r106821090

          — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/functions/windowing/delta/extractor/ArrayFromTuple.java —
          @@ -28,7 +28,7 @@
          public class ArrayFromTuple implements Extractor<Tuple, Object[]> {

          /**

          • * Auto generated version id
            + * Auto generated version id.
              • End diff –

          let's remove this like in ExtractionAwareDeltaFunction

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3567#discussion_r106821090 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/functions/windowing/delta/extractor/ArrayFromTuple.java — @@ -28,7 +28,7 @@ public class ArrayFromTuple implements Extractor<Tuple, Object[]> { /** * Auto generated version id + * Auto generated version id. End diff – let's remove this like in ExtractionAwareDeltaFunction
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3567#discussion_r106821067

          — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/functions/windowing/ReduceIterableWindowFunction.java —
          @@ -1,48 +0,0 @@
          -/**

          • * Licensed to the Apache Software Foundation (ASF) under one
          • * or more contributor license agreements. See the NOTICE file
          • * distributed with this work for additional information
          • * regarding copyright ownership. The ASF licenses this file
          • * to you under the Apache License, Version 2.0 (the
          • * "License"); you may not use this file except in compliance
          • * with the License. You may obtain a copy of the License at
          • *
          • * http://www.apache.org/licenses/LICENSE-2.0
          • *
          • * Unless required by applicable law or agreed to in writing, software
          • * distributed under the License is distributed on an "AS IS" BASIS,
          • * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          • * See the License for the specific language governing permissions and
          • * limitations under the License.
          • */
            -package org.apache.flink.streaming.api.functions.windowing;
            -
            -import org.apache.flink.annotation.Internal;
            -import org.apache.flink.api.common.functions.ReduceFunction;
            -import org.apache.flink.streaming.api.windowing.windows.Window;
            -import org.apache.flink.util.Collector;
            -
            -@Internal
            -public class ReduceIterableWindowFunction<K, W extends Window, T> implements WindowFunction<T, T, K, W> {
              • End diff –

          Unused class?

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3567#discussion_r106821067 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/functions/windowing/ReduceIterableWindowFunction.java — @@ -1,48 +0,0 @@ -/** * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file * distributed with this work for additional information * regarding copyright ownership. The ASF licenses this file * to you under the Apache License, Version 2.0 (the * "License"); you may not use this file except in compliance * with the License. You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.flink.streaming.api.functions.windowing; - -import org.apache.flink.annotation.Internal; -import org.apache.flink.api.common.functions.ReduceFunction; -import org.apache.flink.streaming.api.windowing.windows.Window; -import org.apache.flink.util.Collector; - -@Internal -public class ReduceIterableWindowFunction<K, W extends Window, T> implements WindowFunction<T, T, K, W> { End diff – Unused class?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3567#discussion_r106821059

          — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/functions/windowing/ReduceIterableAllWindowFunction.java —
          @@ -1,48 +0,0 @@
          -/**

          • * Licensed to the Apache Software Foundation (ASF) under one
          • * or more contributor license agreements. See the NOTICE file
          • * distributed with this work for additional information
          • * regarding copyright ownership. The ASF licenses this file
          • * to you under the Apache License, Version 2.0 (the
          • * "License"); you may not use this file except in compliance
          • * with the License. You may obtain a copy of the License at
          • *
          • * http://www.apache.org/licenses/LICENSE-2.0
          • *
          • * Unless required by applicable law or agreed to in writing, software
          • * distributed under the License is distributed on an "AS IS" BASIS,
          • * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          • * See the License for the specific language governing permissions and
          • * limitations under the License.
          • */
            -package org.apache.flink.streaming.api.functions.windowing;
            -
            -import org.apache.flink.annotation.Internal;
            -import org.apache.flink.api.common.functions.ReduceFunction;
            -import org.apache.flink.streaming.api.windowing.windows.Window;
            -import org.apache.flink.util.Collector;
            -
            -@Internal
            -public class ReduceIterableAllWindowFunction<W extends Window, T> implements AllWindowFunction<T, T, W> {
              • End diff –

          Unused class?

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3567#discussion_r106821059 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/functions/windowing/ReduceIterableAllWindowFunction.java — @@ -1,48 +0,0 @@ -/** * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file * distributed with this work for additional information * regarding copyright ownership. The ASF licenses this file * to you under the Apache License, Version 2.0 (the * "License"); you may not use this file except in compliance * with the License. You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.flink.streaming.api.functions.windowing; - -import org.apache.flink.annotation.Internal; -import org.apache.flink.api.common.functions.ReduceFunction; -import org.apache.flink.streaming.api.windowing.windows.Window; -import org.apache.flink.util.Collector; - -@Internal -public class ReduceIterableAllWindowFunction<W extends Window, T> implements AllWindowFunction<T, T, W> { End diff – Unused class?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3567#discussion_r106821137

          — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/io/StreamingReader.java —
          @@ -1,30 +0,0 @@
          -/*

          • * Licensed to the Apache Software Foundation (ASF) under one
          • * or more contributor license agreements. See the NOTICE file
          • * distributed with this work for additional information
          • * regarding copyright ownership. The ASF licenses this file
          • * to you under the Apache License, Version 2.0 (the
          • * "License"); you may not use this file except in compliance
          • * with the License. You may obtain a copy of the License at
          • *
          • * http://www.apache.org/licenses/LICENSE-2.0
          • *
          • * Unless required by applicable law or agreed to in writing, software
          • * distributed under the License is distributed on an "AS IS" BASIS,
          • * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          • * See the License for the specific language governing permissions and
          • * limitations under the License.
          • */
            -
            -package org.apache.flink.streaming.runtime.io;
            -
            -import org.apache.flink.annotation.Internal;
            -import org.apache.flink.runtime.io.network.api.reader.ReaderBase;
            -
            -import java.io.IOException;
            -
            -@Internal
            -public interface StreamingReader extends ReaderBase {
              • End diff –

          Unused class?

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3567#discussion_r106821137 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/io/StreamingReader.java — @@ -1,30 +0,0 @@ -/* * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file * distributed with this work for additional information * regarding copyright ownership. The ASF licenses this file * to you under the Apache License, Version 2.0 (the * "License"); you may not use this file except in compliance * with the License. You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. */ - -package org.apache.flink.streaming.runtime.io; - -import org.apache.flink.annotation.Internal; -import org.apache.flink.runtime.io.network.api.reader.ReaderBase; - -import java.io.IOException; - -@Internal -public interface StreamingReader extends ReaderBase { End diff – Unused class?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3567#discussion_r106820944

          — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/collector/selector/OutputSelectorWrapper.java —
          @@ -1,28 +0,0 @@
          -/*

          • * Licensed to the Apache Software Foundation (ASF) under one or more
          • * contributor license agreements. See the NOTICE file distributed with
          • * this work for additional information regarding copyright ownership.
          • * The ASF licenses this file to You under the Apache License, Version 2.0
          • * (the "License"); you may not use this file except in compliance with
          • * the License. You may obtain a copy of the License at
          • *
          • * http://www.apache.org/licenses/LICENSE-2.0
          • *
          • * Unless required by applicable law or agreed to in writing, software
          • * distributed under the License is distributed on an "AS IS" BASIS,
          • * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          • * See the License for the specific language governing permissions and
          • * limitations under the License.
          • */
            -
            -package org.apache.flink.streaming.api.collector.selector;
            -
            -import java.io.Serializable;
            -
            -import org.apache.flink.annotation.PublicEvolving;
            -
            -@PublicEvolving
            -public interface OutputSelectorWrapper<OUT> extends Serializable {
              • End diff –

          unused class?

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3567#discussion_r106820944 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/collector/selector/OutputSelectorWrapper.java — @@ -1,28 +0,0 @@ -/* * Licensed to the Apache Software Foundation (ASF) under one or more * contributor license agreements. See the NOTICE file distributed with * this work for additional information regarding copyright ownership. * The ASF licenses this file to You under the Apache License, Version 2.0 * (the "License"); you may not use this file except in compliance with * the License. You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. */ - -package org.apache.flink.streaming.api.collector.selector; - -import java.io.Serializable; - -import org.apache.flink.annotation.PublicEvolving; - -@PublicEvolving -public interface OutputSelectorWrapper<OUT> extends Serializable { End diff – unused class?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aljoscha commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3567#discussion_r106853548

          — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/functions/windowing/delta/extractor/ArrayFromTuple.java —
          @@ -28,7 +28,7 @@
          public class ArrayFromTuple implements Extractor<Tuple, Object[]> {

          /**

          • * Auto generated version id
            + * Auto generated version id.
              • End diff –

          dammit, I missed this one...

          Fixing

          Show
          githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on a diff in the pull request: https://github.com/apache/flink/pull/3567#discussion_r106853548 — Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/functions/windowing/delta/extractor/ArrayFromTuple.java — @@ -28,7 +28,7 @@ public class ArrayFromTuple implements Extractor<Tuple, Object[]> { /** * Auto generated version id + * Auto generated version id. End diff – dammit, I missed this one... Fixing
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aljoscha commented on the issue:

          https://github.com/apache/flink/pull/3567

          Thanks for reviewing! 😃

          And yes, the classes I removed where unused.

          Show
          githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/3567 Thanks for reviewing! 😃 And yes, the classes I removed where unused.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3567

          Can you describe what rules this style actually enforces?

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3567 Can you describe what rules this style actually enforces?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on the issue:

          https://github.com/apache/flink/pull/3567

          @StephanEwen It enforeces the following rules:

          • every files has to end with a new-line
          • no trailing whitespace anywhere
          • every public/protected method/class must have a javadoc
          • imports have to be sorted alphabetically
          • static imports must come first
          • whitespace after commas, semicolons and casts
          • whitespace around operators, if, for, etc.
          • left curly brace (`{`) must not be at the start of the line
            ```
            public MyObject(int a)
            {

          =>

          public MyObject(int a)

          { ``` * else/catch/finally must be on the same line as the right curly brace (`}

          `)
          ```
          if {
          }
          else

          =>

          if {
          } else
          ```

          • static final variables must be upper case
          • non-static variables must not be upper case
            ```
            ```
          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/3567 @StephanEwen It enforeces the following rules: every files has to end with a new-line no trailing whitespace anywhere every public/protected method/class must have a javadoc imports have to be sorted alphabetically static imports must come first whitespace after commas, semicolons and casts whitespace around operators, if, for, etc. left curly brace (`{`) must not be at the start of the line ``` public MyObject(int a) { => public MyObject(int a) { ``` * else/catch/finally must be on the same line as the right curly brace (`} `) ``` if { } else => if { } else ``` static final variables must be upper case non-static variables must not be upper case ``` ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aljoscha commented on the issue:

          https://github.com/apache/flink/pull/3567

          In the ML discussion "Google code style" was repeatedly brought up (initially by me): https://lists.apache.org/thread.html/94c8c5186b315c58c3f8aaf536501b99e8b92ee97b0034dee295ff6a@%3Cdev.flink.apache.org%3E.

          I posted this proposal to encourage discussion, that's why I also wrote to the ML again. I think all checks except `RightCurly` and `LeftCurly` are pretty standard Java style so the discussion will probably revolve around those. Or we decide to not enforce a style for those.

          Show
          githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/3567 In the ML discussion "Google code style" was repeatedly brought up (initially by me): https://lists.apache.org/thread.html/94c8c5186b315c58c3f8aaf536501b99e8b92ee97b0034dee295ff6a@%3Cdev.flink.apache.org%3E . I posted this proposal to encourage discussion, that's why I also wrote to the ML again. I think all checks except `RightCurly` and `LeftCurly` are pretty standard Java style so the discussion will probably revolve around those. Or we decide to not enforce a style for those.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user greghogan commented on the issue:

          https://github.com/apache/flink/pull/3567

          @aljoscha do you think we can use a single checkstyle or will this need to be customized per module? Is this enforced by IntelliJ?

          Show
          githubbot ASF GitHub Bot added a comment - Github user greghogan commented on the issue: https://github.com/apache/flink/pull/3567 @aljoscha do you think we can use a single checkstyle or will this need to be customized per module? Is this enforced by IntelliJ?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aljoscha commented on the issue:

          https://github.com/apache/flink/pull/3567

          We will need at least two: the existing checkstyle for all the other modules and the new strict style. (We might get away with having one checkstyle and per-module suppression files that deactivate rules but I think this can lead to more hassle in the end.)

          IntelliJ can be setup to use a custom checkstyle per module. In my setup all the modules use the default Flink checkstyle, only for flink-streaming-java uses the new strict style to mirror the maven setup.

          Show
          githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/3567 We will need at least two: the existing checkstyle for all the other modules and the new strict style. (We might get away with having one checkstyle and per-module suppression files that deactivate rules but I think this can lead to more hassle in the end.) IntelliJ can be setup to use a custom checkstyle per module. In my setup all the modules use the default Flink checkstyle, only for flink-streaming-java uses the new strict style to mirror the maven setup.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user greghogan commented on the issue:

          https://github.com/apache/flink/pull/3567

          @aljoscha should we host the new checkstyle under tools/maven/ alongside the existing checkstyle? There is already a ticket (FLINK-6137) to add a custom checkstyle to flink-cep and I don't see any of these rules being module-specific.

          Show
          githubbot ASF GitHub Bot added a comment - Github user greghogan commented on the issue: https://github.com/apache/flink/pull/3567 @aljoscha should we host the new checkstyle under tools/maven/ alongside the existing checkstyle? There is already a ticket ( FLINK-6137 ) to add a custom checkstyle to flink-cep and I don't see any of these rules being module-specific.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3567

          It would be awesome if we could make IntelliJ pick up the style config
          automatically. By committing some files under ".Idea", we might be able to
          do that...

          On Mar 20, 2017 11:29 PM, "Greg Hogan" <notifications@github.com> wrote:

          > @aljoscha <https://github.com/aljoscha> should we host the new checkstyle
          > under tools/maven/ alongside the existing checkstyle? There is already a
          > ticket (FLINK-6137) to add a custom checkstyle to flink-cep and I don't see
          > any of these rules being module-specific.
          >
          > —
          > You are receiving this because you were mentioned.
          > Reply to this email directly, view it on GitHub
          > <https://github.com/apache/flink/pull/3567#issuecomment-287918204>, or mute
          > the thread
          > <https://github.com/notifications/unsubscribe-auth/ABpaqivIvxg5mrILAz5PQxGwg64Rc021ks5rnv3YgaJpZM4MhxzT>
          > .
          >

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3567 It would be awesome if we could make IntelliJ pick up the style config automatically. By committing some files under ".Idea", we might be able to do that... On Mar 20, 2017 11:29 PM, "Greg Hogan" <notifications@github.com> wrote: > @aljoscha < https://github.com/aljoscha > should we host the new checkstyle > under tools/maven/ alongside the existing checkstyle? There is already a > ticket ( FLINK-6137 ) to add a custom checkstyle to flink-cep and I don't see > any of these rules being module-specific. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > < https://github.com/apache/flink/pull/3567#issuecomment-287918204 >, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/ABpaqivIvxg5mrILAz5PQxGwg64Rc021ks5rnv3YgaJpZM4MhxzT > > . >
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dawidwys commented on the issue:

          https://github.com/apache/flink/pull/3567

          +1 for hosting the checkstyle under tools/maven/

          Show
          githubbot ASF GitHub Bot added a comment - Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/3567 +1 for hosting the checkstyle under tools/maven/
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aljoscha commented on the issue:

          https://github.com/apache/flink/pull/3567

          AFAIK it's not possible to make IntelliJ pick up the checkstyle config by itself. On my machine the setting is in the "*.iml" project file and I think we have so far not included IDE specific project files because this is quite uncommon, I think.

          @dawidwys I'll create a new maven module to host the maven-build related files. This will make it easier to include it in different modules without dealing with paths in the checkstyle config.

          Show
          githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/3567 AFAIK it's not possible to make IntelliJ pick up the checkstyle config by itself. On my machine the setting is in the "*.iml" project file and I think we have so far not included IDE specific project files because this is quite uncommon, I think. @dawidwys I'll create a new maven module to host the maven-build related files. This will make it easier to include it in different modules without dealing with paths in the checkstyle config.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on the issue:

          https://github.com/apache/flink/pull/3567

          Could you create or link a guide on how to configure checkstyles per module in Intellij?

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/3567 Could you create or link a guide on how to configure checkstyles per module in Intellij?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aljoscha commented on the issue:

          https://github.com/apache/flink/pull/3567

          @zentol I added a section to our IDE setup Guide.

          Show
          githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/3567 @zentol I added a section to our IDE setup Guide.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user greghogan commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3567#discussion_r107777860

          — Diff: tools/maven/strict-checkstyle.xml —
          @@ -0,0 +1,550 @@
          +<?xml version="1.0" encoding="UTF-8"?>
          +<!--
          +Licensed to the Apache Software Foundation (ASF) under one
          +or more contributor license agreements. See the NOTICE file
          +distributed with this work for additional information
          +regarding copyright ownership. The ASF licenses this file
          +to you under the Apache License, Version 2.0 (the
          +"License"); you may not use this file except in compliance
          +with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          +Unless required by applicable law or agreed to in writing,
          +software distributed under the License is distributed on an
          +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          +KIND, either express or implied. See the License for the
          +specific language governing permissions and limitations
          +under the License.
          +-->
          +<!DOCTYPE module PUBLIC
          + "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          + "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
          +
          +<!--
          +This is a checkstyle configuration file. For descriptions of
          +what the following rules do, please see the checkstyle configuration
          +page at http://checkstyle.sourceforge.net/config.html.
          +
          +This file is based on the checkstyle file of Apache Beam.
          +-->
          +
          +<module name="Checker">
          +
          + <!-<module name="FileTabCharacter">->
          + <Unable to render embedded object: File (--<) not found.– Checks that there are no tab characters in the file. –>-->
          + <!-</module>->
          +
          + <module name="NewlineAtEndOfFile">
          + <!-- windows can use \n\r vs \n, so enforce the most used one ie UNIx style -->
          + <property name="lineSeparator" value="lf" />
          + </module>
          +
          + <module name="RegexpSingleline">
          + <!-- Checks that TODOs don't have stuff in parenthesis, e.g., username. -->
          + <property name="format" value="((//.)|(*.))TODO(" />
          + <property name="message" value="TODO comments must not include usernames." />
          + <property name="severity" value="error" />
          + </module>
          +
          + <module name="RegexpSingleline">
          + <property name="format" value="\s+$"/>
          + <property name="message" value="Trailing whitespace"/>
          + <property name="severity" value="error"/>
          + </module>
          +
          + <module name="RegexpSingleline">
          + <property name="format" value="Throwables.propagate("/>
          + <property name="message" value="Throwables.propagate is deprecated"/>
          + <property name="severity" value="error"/>
          + </module>
          +
          + <!-- Prevent *Tests.java as tools may not pick them up -->
          + <!-<module name="RegexpOnFilename">->
          + <!-<property name="fileNamePattern" value=".*Tests\.java$" />->
          + <!-</module>->
          +
          + <!-- Allow use of comment to suppress javadocstyle -->
          + <module name="SuppressionCommentFilter">
          + <property name="offCommentFormat" value="CHECKSTYLE.OFF\: ([\w\|]+)"/>
          + <property name="onCommentFormat" value="CHECKSTYLE.ON\: ([\w\|]+)"/>
          + <property name="checkFormat" value="$1"/>
          + </module>
          + <module name="SuppressionFilter">
          + <property name="file" value="$

          {checkstyle.suppressions.file}

          " default="suppressions.xml" />
          + </module>
          +
          + <!-- Check that every module has a package-info.java -->
          + <!-<module name="JavadocPackage"/>->
          +
          + <!--
          +
          + FLINK CUSTOM CHECKS
          +
          + -->
          +
          + <module name="FileLength">
          + <property name="max" value="2500"/>
          + </module>
          +
          + <!-- All Java AST specific tests live under TreeWalker module. -->
          + <module name="TreeWalker">
          +
          + <!--
          +
          + FLINK CUSTOM CHECKS
          +
          + -->
          +
          + <module name="RegexpSinglelineJava">
          + <property name="format" value="^\t* +\t*\S" />
          + <property name="message"
          + value="Line has leading space characters; indentation should be performed with tabs only." />
          + <property name="ignoreComments" value="true" />
          + </module>
          +
          + <!-- Prohibit T.getT() methods for standard boxed types -->
          + <module name="Regexp">
          + <property name="format" value="Boolean\.getBoolean"/>
          + <property name="illegalPattern" value="true"/>
          + <property name="message" value="Use System.getProperties() to get system properties."/>
          — End diff –

          This looks to be the wrong message (same for the two following configurations).

          Show
          githubbot ASF GitHub Bot added a comment - Github user greghogan commented on a diff in the pull request: https://github.com/apache/flink/pull/3567#discussion_r107777860 — Diff: tools/maven/strict-checkstyle.xml — @@ -0,0 +1,550 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> +<!DOCTYPE module PUBLIC + "-//Puppy Crawl//DTD Check Configuration 1.3//EN" + "http://www.puppycrawl.com/dtds/configuration_1_3.dtd"> + +<!-- +This is a checkstyle configuration file. For descriptions of +what the following rules do, please see the checkstyle configuration +page at http://checkstyle.sourceforge.net/config.html . + +This file is based on the checkstyle file of Apache Beam. +--> + +<module name="Checker"> + + <!- <module name="FileTabCharacter"> -> + < Unable to render embedded object: File (--<) not found. – Checks that there are no tab characters in the file. –>--> + <!- </module> -> + + <module name="NewlineAtEndOfFile"> + <!-- windows can use \n\r vs \n, so enforce the most used one ie UNIx style --> + <property name="lineSeparator" value="lf" /> + </module> + + <module name="RegexpSingleline"> + <!-- Checks that TODOs don't have stuff in parenthesis, e.g., username. --> + <property name="format" value="((//. )|(*. ))TODO(" /> + <property name="message" value="TODO comments must not include usernames." /> + <property name="severity" value="error" /> + </module> + + <module name="RegexpSingleline"> + <property name="format" value="\s+$"/> + <property name="message" value="Trailing whitespace"/> + <property name="severity" value="error"/> + </module> + + <module name="RegexpSingleline"> + <property name="format" value="Throwables.propagate("/> + <property name="message" value="Throwables.propagate is deprecated"/> + <property name="severity" value="error"/> + </module> + + <!-- Prevent *Tests.java as tools may not pick them up --> + <!- <module name="RegexpOnFilename"> -> + <!- <property name="fileNamePattern" value=".*Tests\.java$" /> -> + <!- </module> -> + + <!-- Allow use of comment to suppress javadocstyle --> + <module name="SuppressionCommentFilter"> + <property name="offCommentFormat" value="CHECKSTYLE.OFF\: ( [\w\|] +)"/> + <property name="onCommentFormat" value="CHECKSTYLE.ON\: ( [\w\|] +)"/> + <property name="checkFormat" value="$1"/> + </module> + <module name="SuppressionFilter"> + <property name="file" value="$ {checkstyle.suppressions.file} " default="suppressions.xml" /> + </module> + + <!-- Check that every module has a package-info.java --> + <!- <module name="JavadocPackage"/> -> + + <!-- + + FLINK CUSTOM CHECKS + + --> + + <module name="FileLength"> + <property name="max" value="2500"/> + </module> + + <!-- All Java AST specific tests live under TreeWalker module. --> + <module name="TreeWalker"> + + <!-- + + FLINK CUSTOM CHECKS + + --> + + <module name="RegexpSinglelineJava"> + <property name="format" value="^\t* +\t*\S" /> + <property name="message" + value="Line has leading space characters; indentation should be performed with tabs only." /> + <property name="ignoreComments" value="true" /> + </module> + + <!-- Prohibit T.getT() methods for standard boxed types --> + <module name="Regexp"> + <property name="format" value="Boolean\.getBoolean"/> + <property name="illegalPattern" value="true"/> + <property name="message" value="Use System.getProperties() to get system properties."/> — End diff – This looks to be the wrong message (same for the two following configurations).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aljoscha commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3567#discussion_r107946611

          — Diff: tools/maven/strict-checkstyle.xml —
          @@ -0,0 +1,550 @@
          +<?xml version="1.0" encoding="UTF-8"?>
          +<!--
          +Licensed to the Apache Software Foundation (ASF) under one
          +or more contributor license agreements. See the NOTICE file
          +distributed with this work for additional information
          +regarding copyright ownership. The ASF licenses this file
          +to you under the Apache License, Version 2.0 (the
          +"License"); you may not use this file except in compliance
          +with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          +Unless required by applicable law or agreed to in writing,
          +software distributed under the License is distributed on an
          +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          +KIND, either express or implied. See the License for the
          +specific language governing permissions and limitations
          +under the License.
          +-->
          +<!DOCTYPE module PUBLIC
          + "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          + "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
          +
          +<!--
          +This is a checkstyle configuration file. For descriptions of
          +what the following rules do, please see the checkstyle configuration
          +page at http://checkstyle.sourceforge.net/config.html.
          +
          +This file is based on the checkstyle file of Apache Beam.
          +-->
          +
          +<module name="Checker">
          +
          + <!-<module name="FileTabCharacter">->
          + <Unable to render embedded object: File (--<) not found.– Checks that there are no tab characters in the file. –>-->
          + <!-</module>->
          +
          + <module name="NewlineAtEndOfFile">
          + <!-- windows can use \n\r vs \n, so enforce the most used one ie UNIx style -->
          + <property name="lineSeparator" value="lf" />
          + </module>
          +
          + <module name="RegexpSingleline">
          + <!-- Checks that TODOs don't have stuff in parenthesis, e.g., username. -->
          + <property name="format" value="((//.)|(*.))TODO(" />
          + <property name="message" value="TODO comments must not include usernames." />
          + <property name="severity" value="error" />
          + </module>
          +
          + <module name="RegexpSingleline">
          + <property name="format" value="\s+$"/>
          + <property name="message" value="Trailing whitespace"/>
          + <property name="severity" value="error"/>
          + </module>
          +
          + <module name="RegexpSingleline">
          + <property name="format" value="Throwables.propagate("/>
          + <property name="message" value="Throwables.propagate is deprecated"/>
          + <property name="severity" value="error"/>
          + </module>
          +
          + <!-- Prevent *Tests.java as tools may not pick them up -->
          + <!-<module name="RegexpOnFilename">->
          + <!-<property name="fileNamePattern" value=".*Tests\.java$" />->
          + <!-</module>->
          +
          + <!-- Allow use of comment to suppress javadocstyle -->
          + <module name="SuppressionCommentFilter">
          + <property name="offCommentFormat" value="CHECKSTYLE.OFF\: ([\w\|]+)"/>
          + <property name="onCommentFormat" value="CHECKSTYLE.ON\: ([\w\|]+)"/>
          + <property name="checkFormat" value="$1"/>
          + </module>
          + <module name="SuppressionFilter">
          + <property name="file" value="$

          {checkstyle.suppressions.file}

          " default="suppressions.xml" />
          + </module>
          +
          + <!-- Check that every module has a package-info.java -->
          + <!-<module name="JavadocPackage"/>->
          +
          + <!--
          +
          + FLINK CUSTOM CHECKS
          +
          + -->
          +
          + <module name="FileLength">
          + <property name="max" value="2500"/>
          + </module>
          +
          + <!-- All Java AST specific tests live under TreeWalker module. -->
          + <module name="TreeWalker">
          +
          + <!--
          +
          + FLINK CUSTOM CHECKS
          +
          + -->
          +
          + <module name="RegexpSinglelineJava">
          + <property name="format" value="^\t* +\t*\S" />
          + <property name="message"
          + value="Line has leading space characters; indentation should be performed with tabs only." />
          + <property name="ignoreComments" value="true" />
          + </module>
          +
          + <!-- Prohibit T.getT() methods for standard boxed types -->
          + <module name="Regexp">
          + <property name="format" value="Boolean\.getBoolean"/>
          + <property name="illegalPattern" value="true"/>
          + <property name="message" value="Use System.getProperties() to get system properties."/>
          — End diff –

          This is the Javadoc for the method that the checks are referring to:
          ```
          /**

          • Returns {@code true}

            if and only if the system property

          • named by the argument exists and is equal to the string
          • {@code "true"}

            . (Beginning with version 1.0.2 of the

          • Java<small><sup>TM</sup></small> platform, the test of
          • this string is case insensitive.) A system property is accessible
          • through {@code getProperty}

            , a method defined by the

          • {@code System}

            class.

          • <p>
          • If there is no property with the specified name, or if the specified
          • name is empty or null, then {@code false}

            is returned.
            *

          • @param name the system property name.
          • @return the {@code boolean}

            value of the system property.

          • @throws SecurityException for the same reasons as
          • {@link System#getProperty(String) System.getProperty}
          • @see java.lang.System#getProperty(java.lang.String)
          • @see java.lang.System#getProperty(java.lang.String, java.lang.String)
            */
            public static boolean getBoolean(String name) {
            ```
          Show
          githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on a diff in the pull request: https://github.com/apache/flink/pull/3567#discussion_r107946611 — Diff: tools/maven/strict-checkstyle.xml — @@ -0,0 +1,550 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> +<!DOCTYPE module PUBLIC + "-//Puppy Crawl//DTD Check Configuration 1.3//EN" + "http://www.puppycrawl.com/dtds/configuration_1_3.dtd"> + +<!-- +This is a checkstyle configuration file. For descriptions of +what the following rules do, please see the checkstyle configuration +page at http://checkstyle.sourceforge.net/config.html . + +This file is based on the checkstyle file of Apache Beam. +--> + +<module name="Checker"> + + <!- <module name="FileTabCharacter"> -> + < Unable to render embedded object: File (--<) not found. – Checks that there are no tab characters in the file. –>--> + <!- </module> -> + + <module name="NewlineAtEndOfFile"> + <!-- windows can use \n\r vs \n, so enforce the most used one ie UNIx style --> + <property name="lineSeparator" value="lf" /> + </module> + + <module name="RegexpSingleline"> + <!-- Checks that TODOs don't have stuff in parenthesis, e.g., username. --> + <property name="format" value="((//. )|(*. ))TODO(" /> + <property name="message" value="TODO comments must not include usernames." /> + <property name="severity" value="error" /> + </module> + + <module name="RegexpSingleline"> + <property name="format" value="\s+$"/> + <property name="message" value="Trailing whitespace"/> + <property name="severity" value="error"/> + </module> + + <module name="RegexpSingleline"> + <property name="format" value="Throwables.propagate("/> + <property name="message" value="Throwables.propagate is deprecated"/> + <property name="severity" value="error"/> + </module> + + <!-- Prevent *Tests.java as tools may not pick them up --> + <!- <module name="RegexpOnFilename"> -> + <!- <property name="fileNamePattern" value=".*Tests\.java$" /> -> + <!- </module> -> + + <!-- Allow use of comment to suppress javadocstyle --> + <module name="SuppressionCommentFilter"> + <property name="offCommentFormat" value="CHECKSTYLE.OFF\: ( [\w\|] +)"/> + <property name="onCommentFormat" value="CHECKSTYLE.ON\: ( [\w\|] +)"/> + <property name="checkFormat" value="$1"/> + </module> + <module name="SuppressionFilter"> + <property name="file" value="$ {checkstyle.suppressions.file} " default="suppressions.xml" /> + </module> + + <!-- Check that every module has a package-info.java --> + <!- <module name="JavadocPackage"/> -> + + <!-- + + FLINK CUSTOM CHECKS + + --> + + <module name="FileLength"> + <property name="max" value="2500"/> + </module> + + <!-- All Java AST specific tests live under TreeWalker module. --> + <module name="TreeWalker"> + + <!-- + + FLINK CUSTOM CHECKS + + --> + + <module name="RegexpSinglelineJava"> + <property name="format" value="^\t* +\t*\S" /> + <property name="message" + value="Line has leading space characters; indentation should be performed with tabs only." /> + <property name="ignoreComments" value="true" /> + </module> + + <!-- Prohibit T.getT() methods for standard boxed types --> + <module name="Regexp"> + <property name="format" value="Boolean\.getBoolean"/> + <property name="illegalPattern" value="true"/> + <property name="message" value="Use System.getProperties() to get system properties."/> — End diff – This is the Javadoc for the method that the checks are referring to: ``` /** Returns {@code true} if and only if the system property named by the argument exists and is equal to the string {@code "true"} . (Beginning with version 1.0.2 of the Java<small><sup>TM</sup></small> platform, the test of this string is case insensitive.) A system property is accessible through {@code getProperty} , a method defined by the {@code System} class. <p> If there is no property with the specified name, or if the specified name is empty or null, then {@code false} is returned. * @param name the system property name. @return the {@code boolean} value of the system property. @throws SecurityException for the same reasons as {@link System#getProperty(String) System.getProperty} @see java.lang.System#getProperty(java.lang.String) @see java.lang.System#getProperty(java.lang.String, java.lang.String) */ public static boolean getBoolean(String name) { ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aljoscha commented on the issue:

          https://github.com/apache/flink/pull/3567

          I have no preference for any style of import order. I just wanted to mandate some order so that we don't have edit wars when people use different IDE settings.

          @greghogan Have you tried setting up import check settings that more closely match the current flink styl? If we find something that works I'm happy to change that. 😃

          Show
          githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/3567 I have no preference for any style of import order. I just wanted to mandate some order so that we don't have edit wars when people use different IDE settings. @greghogan Have you tried setting up import check settings that more closely match the current flink styl? If we find something that works I'm happy to change that. 😃
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user greghogan commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3567#discussion_r108035621

          — Diff: tools/maven/strict-checkstyle.xml —
          @@ -0,0 +1,550 @@
          +<?xml version="1.0" encoding="UTF-8"?>
          +<!--
          +Licensed to the Apache Software Foundation (ASF) under one
          +or more contributor license agreements. See the NOTICE file
          +distributed with this work for additional information
          +regarding copyright ownership. The ASF licenses this file
          +to you under the Apache License, Version 2.0 (the
          +"License"); you may not use this file except in compliance
          +with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          +Unless required by applicable law or agreed to in writing,
          +software distributed under the License is distributed on an
          +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          +KIND, either express or implied. See the License for the
          +specific language governing permissions and limitations
          +under the License.
          +-->
          +<!DOCTYPE module PUBLIC
          + "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          + "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
          +
          +<!--
          +This is a checkstyle configuration file. For descriptions of
          +what the following rules do, please see the checkstyle configuration
          +page at http://checkstyle.sourceforge.net/config.html.
          +
          +This file is based on the checkstyle file of Apache Beam.
          +-->
          +
          +<module name="Checker">
          +
          + <!-<module name="FileTabCharacter">->
          + <Unable to render embedded object: File (--<) not found.– Checks that there are no tab characters in the file. –>-->
          + <!-</module>->
          +
          + <module name="NewlineAtEndOfFile">
          + <!-- windows can use \n\r vs \n, so enforce the most used one ie UNIx style -->
          + <property name="lineSeparator" value="lf" />
          + </module>
          +
          + <module name="RegexpSingleline">
          + <!-- Checks that TODOs don't have stuff in parenthesis, e.g., username. -->
          + <property name="format" value="((//.)|(*.))TODO(" />
          + <property name="message" value="TODO comments must not include usernames." />
          + <property name="severity" value="error" />
          + </module>
          +
          + <module name="RegexpSingleline">
          + <property name="format" value="\s+$"/>
          + <property name="message" value="Trailing whitespace"/>
          + <property name="severity" value="error"/>
          + </module>
          +
          + <module name="RegexpSingleline">
          + <property name="format" value="Throwables.propagate("/>
          + <property name="message" value="Throwables.propagate is deprecated"/>
          + <property name="severity" value="error"/>
          + </module>
          +
          + <!-- Prevent *Tests.java as tools may not pick them up -->
          + <!-<module name="RegexpOnFilename">->
          + <!-<property name="fileNamePattern" value=".*Tests\.java$" />->
          + <!-</module>->
          +
          + <!-- Allow use of comment to suppress javadocstyle -->
          + <module name="SuppressionCommentFilter">
          + <property name="offCommentFormat" value="CHECKSTYLE.OFF\: ([\w\|]+)"/>
          + <property name="onCommentFormat" value="CHECKSTYLE.ON\: ([\w\|]+)"/>
          + <property name="checkFormat" value="$1"/>
          + </module>
          + <module name="SuppressionFilter">
          + <property name="file" value="$

          {checkstyle.suppressions.file}

          " default="suppressions.xml" />
          + </module>
          +
          + <!-- Check that every module has a package-info.java -->
          + <!-<module name="JavadocPackage"/>->
          +
          + <!--
          +
          + FLINK CUSTOM CHECKS
          +
          + -->
          +
          + <module name="FileLength">
          + <property name="max" value="2500"/>
          + </module>
          +
          + <!-- All Java AST specific tests live under TreeWalker module. -->
          + <module name="TreeWalker">
          +
          + <!--
          +
          + FLINK CUSTOM CHECKS
          +
          + -->
          +
          + <module name="RegexpSinglelineJava">
          + <property name="format" value="^\t* +\t*\S" />
          + <property name="message"
          + value="Line has leading space characters; indentation should be performed with tabs only." />
          + <property name="ignoreComments" value="true" />
          + </module>
          +
          + <!-- Prohibit T.getT() methods for standard boxed types -->
          + <module name="Regexp">
          + <property name="format" value="Boolean\.getBoolean"/>
          + <property name="illegalPattern" value="true"/>
          + <property name="message" value="Use System.getProperties() to get system properties."/>
          — End diff –

          The section comment is `<!-- Prohibit T.getT() methods for standard boxed types -->`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user greghogan commented on a diff in the pull request: https://github.com/apache/flink/pull/3567#discussion_r108035621 — Diff: tools/maven/strict-checkstyle.xml — @@ -0,0 +1,550 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> +<!DOCTYPE module PUBLIC + "-//Puppy Crawl//DTD Check Configuration 1.3//EN" + "http://www.puppycrawl.com/dtds/configuration_1_3.dtd"> + +<!-- +This is a checkstyle configuration file. For descriptions of +what the following rules do, please see the checkstyle configuration +page at http://checkstyle.sourceforge.net/config.html . + +This file is based on the checkstyle file of Apache Beam. +--> + +<module name="Checker"> + + <!- <module name="FileTabCharacter"> -> + < Unable to render embedded object: File (--<) not found. – Checks that there are no tab characters in the file. –>--> + <!- </module> -> + + <module name="NewlineAtEndOfFile"> + <!-- windows can use \n\r vs \n, so enforce the most used one ie UNIx style --> + <property name="lineSeparator" value="lf" /> + </module> + + <module name="RegexpSingleline"> + <!-- Checks that TODOs don't have stuff in parenthesis, e.g., username. --> + <property name="format" value="((//. )|(*. ))TODO(" /> + <property name="message" value="TODO comments must not include usernames." /> + <property name="severity" value="error" /> + </module> + + <module name="RegexpSingleline"> + <property name="format" value="\s+$"/> + <property name="message" value="Trailing whitespace"/> + <property name="severity" value="error"/> + </module> + + <module name="RegexpSingleline"> + <property name="format" value="Throwables.propagate("/> + <property name="message" value="Throwables.propagate is deprecated"/> + <property name="severity" value="error"/> + </module> + + <!-- Prevent *Tests.java as tools may not pick them up --> + <!- <module name="RegexpOnFilename"> -> + <!- <property name="fileNamePattern" value=".*Tests\.java$" /> -> + <!- </module> -> + + <!-- Allow use of comment to suppress javadocstyle --> + <module name="SuppressionCommentFilter"> + <property name="offCommentFormat" value="CHECKSTYLE.OFF\: ( [\w\|] +)"/> + <property name="onCommentFormat" value="CHECKSTYLE.ON\: ( [\w\|] +)"/> + <property name="checkFormat" value="$1"/> + </module> + <module name="SuppressionFilter"> + <property name="file" value="$ {checkstyle.suppressions.file} " default="suppressions.xml" /> + </module> + + <!-- Check that every module has a package-info.java --> + <!- <module name="JavadocPackage"/> -> + + <!-- + + FLINK CUSTOM CHECKS + + --> + + <module name="FileLength"> + <property name="max" value="2500"/> + </module> + + <!-- All Java AST specific tests live under TreeWalker module. --> + <module name="TreeWalker"> + + <!-- + + FLINK CUSTOM CHECKS + + --> + + <module name="RegexpSinglelineJava"> + <property name="format" value="^\t* +\t*\S" /> + <property name="message" + value="Line has leading space characters; indentation should be performed with tabs only." /> + <property name="ignoreComments" value="true" /> + </module> + + <!-- Prohibit T.getT() methods for standard boxed types --> + <module name="Regexp"> + <property name="format" value="Boolean\.getBoolean"/> + <property name="illegalPattern" value="true"/> + <property name="message" value="Use System.getProperties() to get system properties."/> — End diff – The section comment is `<!-- Prohibit T.getT() methods for standard boxed types -->`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user greghogan commented on the issue:

          https://github.com/apache/flink/pull/3567

          @aljoscha, I likewise have no great preference for import order. I do think it is important for the checkstyle to match IntelliJ's code style, either the default or a provided Flink style.

          The default IntelliJ import style can be approximated:

          ```
          <module name="CustomImportOrder">
          <property name="customImportOrderRules" value="THIRD_PARTY_PACKAGE###SPECIAL_IMPORTS###STANDARD_JAVA_PACKAGE###STATIC"/>
          <property name="specialImportsRegExp" value="^javax\."/>
          <property name="standardPackageRegExp" value="^java\."/>
          <property name="sortImportsInGroupAlphabetically" value="true"/>
          <property name="separateLineBetweenGroups" value="false"/>
          </module>
          ```

          That includes a blank line between `javax` and `java` imports. There is an [old ticket with recent activity](https://github.com/checkstyle/checkstyle/issues/525) for this issue to allow blank lines to be explicitly defined.

          That said, I'd go for the Google Style as used in this PR. IntelliJ can import from a checkstyle configuration but I am not seeing any effect from this.

          Show
          githubbot ASF GitHub Bot added a comment - Github user greghogan commented on the issue: https://github.com/apache/flink/pull/3567 @aljoscha, I likewise have no great preference for import order. I do think it is important for the checkstyle to match IntelliJ's code style, either the default or a provided Flink style. The default IntelliJ import style can be approximated: ``` <module name="CustomImportOrder"> <property name="customImportOrderRules" value="THIRD_PARTY_PACKAGE###SPECIAL_IMPORTS###STANDARD_JAVA_PACKAGE###STATIC"/> <property name="specialImportsRegExp" value="^javax\."/> <property name="standardPackageRegExp" value="^java\."/> <property name="sortImportsInGroupAlphabetically" value="true"/> <property name="separateLineBetweenGroups" value="false"/> </module> ``` That includes a blank line between `javax` and `java` imports. There is an [old ticket with recent activity] ( https://github.com/checkstyle/checkstyle/issues/525 ) for this issue to allow blank lines to be explicitly defined. That said, I'd go for the Google Style as used in this PR. IntelliJ can import from a checkstyle configuration but I am not seeing any effect from this.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aljoscha commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3567#discussion_r108058013

          — Diff: tools/maven/strict-checkstyle.xml —
          @@ -0,0 +1,550 @@
          +<?xml version="1.0" encoding="UTF-8"?>
          +<!--
          +Licensed to the Apache Software Foundation (ASF) under one
          +or more contributor license agreements. See the NOTICE file
          +distributed with this work for additional information
          +regarding copyright ownership. The ASF licenses this file
          +to you under the Apache License, Version 2.0 (the
          +"License"); you may not use this file except in compliance
          +with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          +Unless required by applicable law or agreed to in writing,
          +software distributed under the License is distributed on an
          +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          +KIND, either express or implied. See the License for the
          +specific language governing permissions and limitations
          +under the License.
          +-->
          +<!DOCTYPE module PUBLIC
          + "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          + "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
          +
          +<!--
          +This is a checkstyle configuration file. For descriptions of
          +what the following rules do, please see the checkstyle configuration
          +page at http://checkstyle.sourceforge.net/config.html.
          +
          +This file is based on the checkstyle file of Apache Beam.
          +-->
          +
          +<module name="Checker">
          +
          + <!-<module name="FileTabCharacter">->
          + <Unable to render embedded object: File (--<) not found.– Checks that there are no tab characters in the file. –>-->
          + <!-</module>->
          +
          + <module name="NewlineAtEndOfFile">
          + <!-- windows can use \n\r vs \n, so enforce the most used one ie UNIx style -->
          + <property name="lineSeparator" value="lf" />
          + </module>
          +
          + <module name="RegexpSingleline">
          + <!-- Checks that TODOs don't have stuff in parenthesis, e.g., username. -->
          + <property name="format" value="((//.)|(*.))TODO(" />
          + <property name="message" value="TODO comments must not include usernames." />
          + <property name="severity" value="error" />
          + </module>
          +
          + <module name="RegexpSingleline">
          + <property name="format" value="\s+$"/>
          + <property name="message" value="Trailing whitespace"/>
          + <property name="severity" value="error"/>
          + </module>
          +
          + <module name="RegexpSingleline">
          + <property name="format" value="Throwables.propagate("/>
          + <property name="message" value="Throwables.propagate is deprecated"/>
          + <property name="severity" value="error"/>
          + </module>
          +
          + <!-- Prevent *Tests.java as tools may not pick them up -->
          + <!-<module name="RegexpOnFilename">->
          + <!-<property name="fileNamePattern" value=".*Tests\.java$" />->
          + <!-</module>->
          +
          + <!-- Allow use of comment to suppress javadocstyle -->
          + <module name="SuppressionCommentFilter">
          + <property name="offCommentFormat" value="CHECKSTYLE.OFF\: ([\w\|]+)"/>
          + <property name="onCommentFormat" value="CHECKSTYLE.ON\: ([\w\|]+)"/>
          + <property name="checkFormat" value="$1"/>
          + </module>
          + <module name="SuppressionFilter">
          + <property name="file" value="$

          {checkstyle.suppressions.file}

          " default="suppressions.xml" />
          + </module>
          +
          + <!-- Check that every module has a package-info.java -->
          + <!-<module name="JavadocPackage"/>->
          +
          + <!--
          +
          + FLINK CUSTOM CHECKS
          +
          + -->
          +
          + <module name="FileLength">
          + <property name="max" value="2500"/>
          + </module>
          +
          + <!-- All Java AST specific tests live under TreeWalker module. -->
          + <module name="TreeWalker">
          +
          + <!--
          +
          + FLINK CUSTOM CHECKS
          +
          + -->
          +
          + <module name="RegexpSinglelineJava">
          + <property name="format" value="^\t* +\t*\S" />
          + <property name="message"
          + value="Line has leading space characters; indentation should be performed with tabs only." />
          + <property name="ignoreComments" value="true" />
          + </module>
          +
          + <!-- Prohibit T.getT() methods for standard boxed types -->
          + <module name="Regexp">
          + <property name="format" value="Boolean\.getBoolean"/>
          + <property name="illegalPattern" value="true"/>
          + <property name="message" value="Use System.getProperties() to get system properties."/>
          — End diff –

          Ah, I should have mentioned that the Javadoc is for `Boolean.getBoolean()`. I copied these checks from our existing Flink checkstyle and I think the reason for prohibiting these is that it's very easy to confuse `Boolean.getBoolean(String)`, which reads from system properties, and `Boolean.parseBoolean(String)`, which parses the given string as a boolean. That's the reason behind the somewhat strange message which tells you to use `System.getProperties()` instead.

          Show
          githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on a diff in the pull request: https://github.com/apache/flink/pull/3567#discussion_r108058013 — Diff: tools/maven/strict-checkstyle.xml — @@ -0,0 +1,550 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> +<!DOCTYPE module PUBLIC + "-//Puppy Crawl//DTD Check Configuration 1.3//EN" + "http://www.puppycrawl.com/dtds/configuration_1_3.dtd"> + +<!-- +This is a checkstyle configuration file. For descriptions of +what the following rules do, please see the checkstyle configuration +page at http://checkstyle.sourceforge.net/config.html . + +This file is based on the checkstyle file of Apache Beam. +--> + +<module name="Checker"> + + <!- <module name="FileTabCharacter"> -> + < Unable to render embedded object: File (--<) not found. – Checks that there are no tab characters in the file. –>--> + <!- </module> -> + + <module name="NewlineAtEndOfFile"> + <!-- windows can use \n\r vs \n, so enforce the most used one ie UNIx style --> + <property name="lineSeparator" value="lf" /> + </module> + + <module name="RegexpSingleline"> + <!-- Checks that TODOs don't have stuff in parenthesis, e.g., username. --> + <property name="format" value="((//. )|(*. ))TODO(" /> + <property name="message" value="TODO comments must not include usernames." /> + <property name="severity" value="error" /> + </module> + + <module name="RegexpSingleline"> + <property name="format" value="\s+$"/> + <property name="message" value="Trailing whitespace"/> + <property name="severity" value="error"/> + </module> + + <module name="RegexpSingleline"> + <property name="format" value="Throwables.propagate("/> + <property name="message" value="Throwables.propagate is deprecated"/> + <property name="severity" value="error"/> + </module> + + <!-- Prevent *Tests.java as tools may not pick them up --> + <!- <module name="RegexpOnFilename"> -> + <!- <property name="fileNamePattern" value=".*Tests\.java$" /> -> + <!- </module> -> + + <!-- Allow use of comment to suppress javadocstyle --> + <module name="SuppressionCommentFilter"> + <property name="offCommentFormat" value="CHECKSTYLE.OFF\: ( [\w\|] +)"/> + <property name="onCommentFormat" value="CHECKSTYLE.ON\: ( [\w\|] +)"/> + <property name="checkFormat" value="$1"/> + </module> + <module name="SuppressionFilter"> + <property name="file" value="$ {checkstyle.suppressions.file} " default="suppressions.xml" /> + </module> + + <!-- Check that every module has a package-info.java --> + <!- <module name="JavadocPackage"/> -> + + <!-- + + FLINK CUSTOM CHECKS + + --> + + <module name="FileLength"> + <property name="max" value="2500"/> + </module> + + <!-- All Java AST specific tests live under TreeWalker module. --> + <module name="TreeWalker"> + + <!-- + + FLINK CUSTOM CHECKS + + --> + + <module name="RegexpSinglelineJava"> + <property name="format" value="^\t* +\t*\S" /> + <property name="message" + value="Line has leading space characters; indentation should be performed with tabs only." /> + <property name="ignoreComments" value="true" /> + </module> + + <!-- Prohibit T.getT() methods for standard boxed types --> + <module name="Regexp"> + <property name="format" value="Boolean\.getBoolean"/> + <property name="illegalPattern" value="true"/> + <property name="message" value="Use System.getProperties() to get system properties."/> — End diff – Ah, I should have mentioned that the Javadoc is for `Boolean.getBoolean()`. I copied these checks from our existing Flink checkstyle and I think the reason for prohibiting these is that it's very easy to confuse `Boolean.getBoolean(String)`, which reads from system properties, and `Boolean.parseBoolean(String)`, which parses the given string as a boolean. That's the reason behind the somewhat strange message which tells you to use `System.getProperties()` instead.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aljoscha commented on the issue:

          https://github.com/apache/flink/pull/3567

          @greghogan Now I'm confused. 😅 Are you suggesting we use the Google style or switch to the default IntelliJ style?

          Show
          githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/3567 @greghogan Now I'm confused. 😅 Are you suggesting we use the Google style or switch to the default IntelliJ style?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user greghogan commented on the issue:

          https://github.com/apache/flink/pull/3567

          Google's style seems sensible since IntelliJ's style cannot be modeled in Checkstyle, the import listing is folded by default, and developers will need to load other non-default configuration settings into IntelliJ.

          Show
          githubbot ASF GitHub Bot added a comment - Github user greghogan commented on the issue: https://github.com/apache/flink/pull/3567 Google's style seems sensible since IntelliJ's style cannot be modeled in Checkstyle, the import listing is folded by default, and developers will need to load other non-default configuration settings into IntelliJ.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on the issue:

          https://github.com/apache/flink/pull/3567

          @aljoscha i believe enough earth rotations have passed

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/3567 @aljoscha i believe enough earth rotations have passed
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/3567

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3567
          Hide
          aljoscha Aljoscha Krettek added a comment -

          Implemented in 80d1c06dc04937a47e0853309e0add63903023b8

          Show
          aljoscha Aljoscha Krettek added a comment - Implemented in 80d1c06dc04937a47e0853309e0add63903023b8
          Hide
          StephanEwen Stephan Ewen added a comment -

          I would like to make a suggestion for an adjustment to the style, specifically to the import order.

          Basically all other files in Flink have the pattern:

          1. Flink / library imports
          2. java.foo.bar imports
          3. static imports

          Also, most files place spaces between components that the imports are derived from.

          It may just be me, being an oldschool guy that looks at the imports quite a bit (almost for every new class I open, I find it useful to get an initial overview of what a class interacts with), but I find the style before was much better to get a "quick glance" impression:

          • Spaces between logical goups (Flink / logger / library / etc) and
          • Flink and libraries first (its what matters to get the overview)
          • java below (not really important for the overview)
          • static imports last (they are just syntactic sugar and not required for any understanting).

          So, why don't we keep that style? It would also result in fewer necessary reformatting and fewer merge conflicts.
          I pasted the examples below to illustrate that:

          Original formatting of most files

          import org.apache.flink.annotation.Internal;
          import org.apache.flink.runtime.checkpoint.CheckpointMetaData;
          import org.apache.flink.runtime.checkpoint.CheckpointMetrics;
          import org.apache.flink.runtime.io.disk.iomanager.IOManager;
          import org.apache.flink.runtime.io.network.api.CancelCheckpointMarker;
          import org.apache.flink.runtime.io.network.api.CheckpointBarrier;
          import org.apache.flink.runtime.io.network.api.EndOfPartitionEvent;
          import org.apache.flink.runtime.io.network.partition.consumer.BufferOrEvent;
          import org.apache.flink.runtime.io.network.partition.consumer.InputGate;
          import org.apache.flink.runtime.jobgraph.tasks.StatefulTask;
          
          import org.slf4j.Logger;
          import org.slf4j.LoggerFactory;
          
          import java.io.IOException;
          import java.util.ArrayDeque;
          
          import static org.apache.flink.util.Preconditions.checkArgument;
          

          New Format

          import static org.apache.flink.util.Preconditions.checkArgument;
          
          import java.io.IOException;
          import java.util.ArrayDeque;
          import org.apache.flink.annotation.Internal;
          import org.apache.flink.runtime.checkpoint.CheckpointMetaData;
          import org.apache.flink.runtime.checkpoint.CheckpointMetrics;
          import org.apache.flink.runtime.io.disk.iomanager.IOManager;
          import org.apache.flink.runtime.io.network.api.CancelCheckpointMarker;
          import org.apache.flink.runtime.io.network.api.CheckpointBarrier;
          import org.apache.flink.runtime.io.network.api.EndOfPartitionEvent;
          import org.apache.flink.runtime.io.network.partition.consumer.BufferOrEvent;
          import org.apache.flink.runtime.io.network.partition.consumer.InputGate;
          import org.apache.flink.runtime.jobgraph.tasks.StatefulTask;
          import org.slf4j.Logger;
          import org.slf4j.LoggerFactory;
          
          Show
          StephanEwen Stephan Ewen added a comment - I would like to make a suggestion for an adjustment to the style, specifically to the import order. Basically all other files in Flink have the pattern: Flink / library imports java.foo.bar imports static imports Also, most files place spaces between components that the imports are derived from. It may just be me, being an oldschool guy that looks at the imports quite a bit (almost for every new class I open, I find it useful to get an initial overview of what a class interacts with), but I find the style before was much better to get a "quick glance" impression: Spaces between logical goups (Flink / logger / library / etc) and Flink and libraries first (its what matters to get the overview) java below (not really important for the overview) static imports last (they are just syntactic sugar and not required for any understanting). So, why don't we keep that style? It would also result in fewer necessary reformatting and fewer merge conflicts. I pasted the examples below to illustrate that: Original formatting of most files import org.apache.flink.annotation.Internal; import org.apache.flink.runtime.checkpoint.CheckpointMetaData; import org.apache.flink.runtime.checkpoint.CheckpointMetrics; import org.apache.flink.runtime.io.disk.iomanager.IOManager; import org.apache.flink.runtime.io.network.api.CancelCheckpointMarker; import org.apache.flink.runtime.io.network.api.CheckpointBarrier; import org.apache.flink.runtime.io.network.api.EndOfPartitionEvent; import org.apache.flink.runtime.io.network.partition.consumer.BufferOrEvent; import org.apache.flink.runtime.io.network.partition.consumer.InputGate; import org.apache.flink.runtime.jobgraph.tasks.StatefulTask; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; import java.util.ArrayDeque; import static org.apache.flink.util.Preconditions.checkArgument; New Format import static org.apache.flink.util.Preconditions.checkArgument; import java.io.IOException; import java.util.ArrayDeque; import org.apache.flink.annotation.Internal; import org.apache.flink.runtime.checkpoint.CheckpointMetaData; import org.apache.flink.runtime.checkpoint.CheckpointMetrics; import org.apache.flink.runtime.io.disk.iomanager.IOManager; import org.apache.flink.runtime.io.network.api.CancelCheckpointMarker; import org.apache.flink.runtime.io.network.api.CheckpointBarrier; import org.apache.flink.runtime.io.network.api.EndOfPartitionEvent; import org.apache.flink.runtime.io.network.partition.consumer.BufferOrEvent; import org.apache.flink.runtime.io.network.partition.consumer.InputGate; import org.apache.flink.runtime.jobgraph.tasks.StatefulTask; import org.slf4j.Logger; import org.slf4j.LoggerFactory;
          Hide
          greghogan Greg Hogan added a comment -

          Stephan Ewen, I think there was a perception that import order was not as consistent throughout the project. Previously there didn't seem to be strong opinions either way. I suspect the import order has drifted out-of-sync from your original work because this has not been enforced.

          Show
          greghogan Greg Hogan added a comment - Stephan Ewen , I think there was a perception that import order was not as consistent throughout the project. Previously there didn't seem to be strong opinions either way. I suspect the import order has drifted out-of-sync from your original work because this has not been enforced.
          Hide
          Zentol Chesnay Schepler added a comment -

          The import statements were also frequently being messed with in PR's; adding new entries that aren't in alphabetical order, reordering imports (like putting java imports up top) or removing the separating empty lines.

          Given that (i guess?) few people consider import statements as something functional this was a good opportunity to use the same rules as another well-established checkstyle.

          I'm also not sure how well we could describe the existing scheme :/

          Show
          Zentol Chesnay Schepler added a comment - The import statements were also frequently being messed with in PR's; adding new entries that aren't in alphabetical order, reordering imports (like putting java imports up top) or removing the separating empty lines. Given that (i guess?) few people consider import statements as something functional this was a good opportunity to use the same rules as another well-established checkstyle. I'm also not sure how well we could describe the existing scheme :/
          Hide
          aljoscha Aljoscha Krettek added a comment -

          Chiming in a bit late, sorry for that. I had (and have) no preference regarding the actual import order/layout. I just added a check for something that is easy to check and easy to set in the IDE to present back-and-forth changes in the imports.

          Show
          aljoscha Aljoscha Krettek added a comment - Chiming in a bit late, sorry for that. I had (and have) no preference regarding the actual import order/layout. I just added a check for something that is easy to check and easy to set in the IDE to present back-and-forth changes in the imports.
          Hide
          StephanEwen Stephan Ewen added a comment -

          I agree that we want an import checkstyle (as Chesnay Schepler mentioned, it helps will pull requests).

          If no one (except maybe me) has any preference for the import styles, would anyone mind updating it? I'll volunteer to figure out over the next week how to implement these rules...

          Show
          StephanEwen Stephan Ewen added a comment - I agree that we want an import checkstyle (as Chesnay Schepler mentioned, it helps will pull requests). If no one (except maybe me) has any preference for the import styles, would anyone mind updating it? I'll volunteer to figure out over the next week how to implement these rules...
          Hide
          aljoscha Aljoscha Krettek added a comment -

          +1

          Show
          aljoscha Aljoscha Krettek added a comment - +1
          Hide
          Zentol Chesnay Schepler added a comment -

          I can see the benefits of less merge conflicts (at the expense of the poor souls that have to rebase across both import orders); and i do prefer the older import order.

          That said I'm apprehensive to give a +1, because we had a near 2 month long discussion on the mailing, with an open PR, giving everyone plenty of opportunities to see the required changes and voice concerns. This change didn't come out of nowhere, and i have a hard time justifying, for myself, the reversal of something that has been proposed from the very start.

          Show
          Zentol Chesnay Schepler added a comment - I can see the benefits of less merge conflicts (at the expense of the poor souls that have to rebase across both import orders); and i do prefer the older import order. That said I'm apprehensive to give a +1, because we had a near 2 month long discussion on the mailing, with an open PR, giving everyone plenty of opportunities to see the required changes and voice concerns. This change didn't come out of nowhere, and i have a hard time justifying, for myself, the reversal of something that has been proposed from the very start.
          Hide
          greghogan Greg Hogan added a comment -

          We justify pragmatic changes to earlier decisions quite readily, such as the postponed "time-based" code freeze and release. Now is the best time to reevaluate any checkstyle change before developers adapt and additional modules are updated.

          I'd volunteer but I think FLINK-4370 should be resolved first so that we can pair enforced checkstyle with importable IDE configurations.

          Show
          greghogan Greg Hogan added a comment - We justify pragmatic changes to earlier decisions quite readily, such as the postponed "time-based" code freeze and release. Now is the best time to reevaluate any checkstyle change before developers adapt and additional modules are updated. I'd volunteer but I think FLINK-4370 should be resolved first so that we can pair enforced checkstyle with importable IDE configurations.
          Hide
          StephanEwen Stephan Ewen added a comment -

          I also thought that the checkstyle on flink-streaming-java is explicitly a test bed (one project first, see how it plays out). It was just feedback on how it played out for me...

          Greg also has a point in that decisions should never be final if something important comes up (in general, not saying my feedback was super important )

          Show
          StephanEwen Stephan Ewen added a comment - I also thought that the checkstyle on flink-streaming-java is explicitly a test bed (one project first, see how it plays out). It was just feedback on how it played out for me... Greg also has a point in that decisions should never be final if something important comes up (in general, not saying my feedback was super important )

            People

            • Assignee:
              aljoscha Aljoscha Krettek
              Reporter:
              aljoscha Aljoscha Krettek
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development