[GitHub] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
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 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 issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
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 necessary. --- 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 issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
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 package to make the reviewing easier imo. --- 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 issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
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 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 issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
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 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 issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
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 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 issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
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 else; so far i relied on failing tests to tell me that. --- 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 issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
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 be private` --- 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 issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
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. --- 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 issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
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 `MinAggregationFunction.java`. Is it safe to do so? Can I check it does not interfere with any part of the system. Or should I just revert it and add appropriate javadoc? @zentol What is your opinion? --- 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. ---
