[GitHub] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java

2017-07-13 Thread dawidwys
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/4009 Hi @greghogan @zentol I will rebase this PR, fix remaining checkstyle violations and split into PRs corresponding to packages. Is that ok? --- If your project is set up for it, you can reply to thi

[GitHub] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java

2017-07-12 Thread greghogan
Github user greghogan commented on the issue: https://github.com/apache/flink/pull/4009 Okay, I now see there are many outstanding violations which look to be the type fixable by regex. If we split out PRs by package we should by able to copy over the comments updates from this PR as

[GitHub] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java

2017-07-12 Thread zentol
Github user zentol commented on the issue: https://github.com/apache/flink/pull/4009 I was wondering whether we want to extend this PR to cover all checkstyle rules at once. We are touching so much already, might as well go all-in. We should also try to group the changes by pa

[GitHub] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java

2017-07-12 Thread greghogan
Github user greghogan commented on the issue: https://github.com/apache/flink/pull/4009 @zentol I believe this is the only outstanding checkstyle PR. Is this still in your review queue? --- If your project is set up for it, you can reply to this email and have your reply appear on Gi

[GitHub] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java

2017-05-29 Thread zentol
Github user zentol commented on the issue: https://github.com/apache/flink/pull/4009 I've noticed some `` tags here and there, could replace those as well. --- 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

[GitHub] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java

2017-05-29 Thread dawidwys
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/4009 Yes sorry for that, Just wanted to remove the `AvoidStarImport` commit from in the middle. Any further changes will do with new commits. --- If your project is set up for it, you can reply to this

[GitHub] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java

2017-05-29 Thread zentol
Github user zentol commented on the issue: https://github.com/apache/flink/pull/4009 eh...the commits just jumped around so i assume you just force pushed? From this point on please don't. As for kryo, well you got me there. Frankly i don't know where we use kryo or something

[GitHub] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java

2017-05-29 Thread dawidwys
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/4009 Right, but won't there be any serialization issues? I found a comment like which made me ask that question: `// Nested classes are only "public static" for Kryo serialization, otherwise they'd b

[GitHub] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java

2017-05-29 Thread zentol
Github user zentol commented on the issue: https://github.com/apache/flink/pull/4009 If a function is tagged as `@Internal` or in a class tagged as such then we should be able to restrict the visibility. Would be good if you could reply in the JIRA for the tuple checkstyle exclusion.

[GitHub] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java

2017-05-29 Thread dawidwys
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/4009 I've created set of changes that remove ~3k checkstyle violations in flink-java. I had doubts about changing some `public static final classes` into `private static final classes` e.g in `