It’s unusual to vote on a code change. We could just have a discussion, and reach consensus about whether the PR is a net benefit.
-1 > On Dec 4, 2019, at 2:30 AM, Vladimir Sitnikov <[email protected]> > wrote: > > 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
