[GitHub] flink pull request #3567: [FLINK-6107] Add custom checkstyle for flink-strea...

[asfgit]

Github user asfgit closed the pull request at:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #3567: [FLINK-6107] Add custom checkstyle for flink-strea...

[aljoscha]

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 @@
+
+
+http://www.puppycrawl.com/dtds/configuration_1_3.dtd;>
+
+
+
+
+
+  
+
+  
+
+  
+
+
+  
+
+  
+
+
+
+
+  
+
+  
+
+
+
+  
+
+  
+
+
+
+  
+
+  
+  
+
+  
+
+  
+  
+
+
+
+  
+  
+
+  
+
+  
+  
+
+  
+
+  
+
+  
+
+  
+  
+
+
+
+
+  
+  
+  
+
+
+
+
+  
+  
+  
--- 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. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #3567: [FLINK-6107] Add custom checkstyle for flink-strea...

[greghogan]

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 @@
+
+
+http://www.puppycrawl.com/dtds/configuration_1_3.dtd;>
+
+
+
+
+
+  
+
+  
+
+  
+
+
+  
+
+  
+
+
+
+
+  
+
+  
+
+
+
+  
+
+  
+
+
+
+  
+
+  
+  
+
+  
+
+  
+  
+
+
+
+  
+  
+
+  
+
+  
+  
+
+  
+
+  
+
+  
+
+  
+  
+
+
+
+
+  
+  
+  
+
+
+
+
+  
+  
+  
--- End diff --

The section comment is ``.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #3567: [FLINK-6107] Add custom checkstyle for flink-strea...

[aljoscha]

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 @@
+
+
+http://www.puppycrawl.com/dtds/configuration_1_3.dtd;>
+
+
+
+
+
+  
+
+  
+
+  
+
+
+  
+
+  
+
+
+
+
+  
+
+  
+
+
+
+  
+
+  
+
+
+
+  
+
+  
+  
+
+  
+
+  
+  
+
+
+
+  
+  
+
+  
+
+  
+  
+
+  
+
+  
+
+  
+
+  
+  
+
+
+
+
+  
+  
+  
+
+
+
+
+  
+  
+  
--- 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
 * JavaTM 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.
 * 
 * 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) {
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #3567: [FLINK-6107] Add custom checkstyle for flink-strea...

[greghogan]

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

https://github.com/apache/flink/pull/3567#discussion_r10860
  
--- Diff: tools/maven/strict-checkstyle.xml ---
@@ -0,0 +1,550 @@
+
+
+http://www.puppycrawl.com/dtds/configuration_1_3.dtd;>
+
+
+
+
+
+  
+
+  
+
+  
+
+
+  
+
+  
+
+
+
+
+  
+
+  
+
+
+
+  
+
+  
+
+
+
+  
+
+  
+  
+
+  
+
+  
+  
+
+
+
+  
+  
+
+  
+
+  
+  
+
+  
+
+  
+
+  
+
+  
+  
+
+
+
+
+  
+  
+  
+
+
+
+
+  
+  
+  
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #3567: [FLINK-6107] Add custom checkstyle for flink-strea...

[aljoscha]

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 {
 
/**
-* Auto generated version id
+* Auto generated version id.
--- End diff --

dammit, I missed this one...

Fixing


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #3567: [FLINK-6107] Add custom checkstyle for flink-strea...

[zentol]

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 {
 
/**
-* Auto generated version id
+* Auto generated version id.
--- End diff --

let's remove this like in ExtractionAwareDeltaFunction


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #3567: [FLINK-6107] Add custom checkstyle for flink-strea...

[zentol]

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 
implements AllWindowFunction {
--- End diff --

Unused class?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #3567: [FLINK-6107] Add custom checkstyle for flink-strea...

[zentol]

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 
implements WindowFunction {
--- End diff --

Unused class?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #3567: [FLINK-6107] Add custom checkstyle for flink-strea...

[zentol]

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?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #3567: [FLINK-6107] Add custom checkstyle for flink-strea...

[zentol]

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 extends Serializable {
--- End diff --

unused class?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #3567: [FLINK-6107] Add custom checkstyle for flink-strea...

[aljoscha]

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 
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 
Date:   2017-03-19T09:29:10Z

[FLINK-6107] Enable newline at EOF check in streaming checkstyle

commit 42306cffcd0082ea5077892fb84e40371ba81377
Author: Aljoscha Krettek 
Date:   2017-03-19T10:17:47Z

[FLINK-6107] Enable trailing whitespace check in streaming checkstyle

commit e8dd1759d059ac138b7a528ef4775217fbf55742
Author: Aljoscha Krettek 
Date:   2017-03-19T10:21:17Z

[FLINK-6107] Enable Javadoc checks in streaming checkstyle

commit 4cd343c03fcbef3c8115005346d03a583e467849
Author: Aljoscha Krettek 
Date:   2017-03-19T11:23:22Z

[FLINK-6107] Enable import order check in streaming checkstyle

commit bbc7d75bc692656707b70b844b044e71259d148a
Author: Aljoscha Krettek 
Date:   2017-03-19T13:54:35Z

[FLINK-6107] Enable WhitespaceAfter check in streaming checkstyle

commit ccb3e28a66d17daee08bb71c89cdaf82fefc412b
Author: Aljoscha Krettek 
Date:   2017-03-19T14:02:00Z

[FLINK-6107] Enable WhitespaceAround check in streaming checkstyle

commit 66f8f9b47f646ddaf2a333ec68ba51edac306c16
Author: Aljoscha Krettek 
Date:   2017-03-19T14:18:21Z

[FLINK-6107] Enable RightCurly check in streaming checkstyle

commit e061edd14db4e3af6844697194aee78324170508
Author: Aljoscha Krettek 
Date:   2017-03-19T14:25:24Z

[FLINK-6107] Enable LeftCurly check in streaming checkstyle

commit a88fb33dc02781e04030d7c91351659937d3a836
Author: Aljoscha Krettek 
Date:   2017-03-19T14:29:02Z

[FLINK-6107] Enable MemberNameCheck in streaming checkstyle

commit 181d8b3649293b143106fe66d6a3b5c6b3dbaf93
Author: Aljoscha Krettek 
Date:   2017-03-19T14:30:23Z

[FLINK-6107] Enable StaticVariableNameCheck in streaming checkstyle




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature