Hi, I suggest we drop HydromaticFileSetCheck from the Checkstyle configuration. It was a great journey (thanks Julian for HydromaticFileSetCheck), however, lots of tools have improved since then, so we could upgrade to the better tooling now.
[ ] +1, drop HydromaticFileSetCheck, just do it [ ] -1, do not drop it because... The vote is open for 120 hours (till 2019-12-09 11:00:00 UTC) Motivation: * Current Checkstyle 7.x has concurrency issues, and it produces wrong results when multiple modules are checked concurrently * HydromaticFileSetCheck blocks upgrade to Checkstyle 8 * HydromaticFileSetCheck seems to cause issues in Eclipse (see CALCITE-1127) * HydromaticFileSetCheck is not supported <https://github.com/julianhyde/toolbox>: the latest commit was 3-4 years ago, trivial issues are not resolved since July, trivial pull request receives no comments for more than 15 days * HydromaticFileSetCheck produces hard to understand messages. I guess many of you have faced "Open parentheses exceed closes by 2 or more [HydromaticFileSet]" issue, and I guess you it took you a lot to understand what the error really means. * HydromaticFileSetCheck duplicates logic which is already implemented elsewhere (e.g. fail on tab characters, end with newline) * HydromaticFileSetCheck produces errors when automatic correction is possible. For instance, "@Override should not be on its own line" * "// End ..." file footer is redundant, and sometimes it is misleading (e.g. // End Parser.jj is retained even in the generated parser which makes no sense). Solution: https://github.com/apache/calcite/pull/1625 Please check the PR description and a showcase for parenthesis <https://github.com/apache/calcite/pull/1625#issuecomment-561563891> for more info. In a nutshell, it implements automatic fixes, so the build produces easier to understand errors, and the fixes could be applied automatically. The PR discovers a few violations (~10) that were never detected before. For instance (even though it looks like a diff, it is a copy-paste of the build script output where it complains on the wrong formatting and it suggests the fix): core/src/main/java/org/apache/calcite/rel/rules/AggregateValuesRule.java -········literals.add((RexLiteral)·rexBuilder.makeLiteral( +········literals.add( +············(RexLiteral)·rexBuilder.makeLiteral( ············BigDecimal.ZERO,·aggregateCall.getType(),·false)); kafka/src/main/java/org/apache/calcite/adapter/kafka/KafkaTableFactory.java ····if·(operand.containsKey(KafkaTableConstants.SCHEMA_CONSUMER_PARAMS))·{ -······tableOptionBuilder.setConsumerParams((Map<String,·String>)·operand.get( +······tableOptionBuilder.setConsumerParams( +··········(Map<String,·String>)·operand.get( ··········KafkaTableConstants.SCHEMA_CONSUMER_PARAMS)); ····} I'll fix the violations as a part of the PR as well. Note: in the both cases I've provided above the autofix is suboptimal, however, I guess does convey the intended formatting. Vladimir
