rewrite icon indicating copy to clipboard operation
rewrite copied to clipboard

Add rewrite rule: parenthesis balance

Open vlsi opened this issue 4 years ago • 1 comments

The parenthesis-heavy code is often hard to reason about.

For instance:

literals.add((RexLiteral) rexBuilder.makeLiteral(
    BigDecimal.ZERO, aggregateCall.getType(), false));

Even though this code might look OK, there's a readability issue. It is not clear if BigDecimal.ZERO is an argument to .add method or .makeLiteral method.

The very same code can be reformatted as

literals.add(
    (RexLiteral) rexBuilder.makeLiteral(
        BigDecimal.ZERO, aggregateCall.getType(), false));

or as

literals.add(
    (RexLiteral) rexBuilder.makeLiteral(
        BigDecimal.ZERO,
        aggregateCall.getType(),
        false));

Both variations make it clear that .add takes a single argument. The observation is that the code should open at most one parenthesis on a single line.

Here's a regexp-driven implementation, however, what it does it finds a line which opens more than one unclosed parenthesis (e.g. eq(gt(x,), but it can't re-format the full expression with new indentation.

Note: eq(gt(x,2),3) is not an issue because all the open parenthesis are closed on the same line


Other samples of undesired code (see https://github.com/autostyle/autostyle/issues/6 ):

    if (!(group.isRows || (group.upperBound.isUnbounded()
        && group.lowerBound.isUnbounded()))) {
    builder.addRuleCollection(ImmutableList.of((RelOptRule) SubQueryRemoveRule.FILTER,
        SubQueryRemoveRule.PROJECT,
        SubQueryRemoveRule.JOIN));
      if (((nlsString.getCharset() != null
          && type.getCharset().equals(nlsString.getCharset()))
          || (nlsString.getCharset() == null
          && SqlCollation.IMPLICIT.getCharset().equals(type.getCharset())))
          && nlsString.getCollation().equals(type.getCollation())
          && ((NlsString) value).getValue().length() == type.getPrecision()) {
        includeType = RexDigestIncludeType.NO_TYPE;
      } else {
        includeType = RexDigestIncludeType.ALWAYS;
      }
    if ((leftRowCount != null)
        && (rightRowCount != null)
        && ((leftRowCount < rightRowCount)
        || ((Math.abs(leftRowCount - rightRowCount)
        < RelOptUtil.EPSILON)
        && (rowWidthCost(left.getJoinTree())
        < rowWidthCost(right.getJoinTree()))))) {
      swap = true;
    }
    Object expr4 =
        gt(
            qw(42, 43), eq(gt(x,
                y), 2));

    Object expr = gt(eq(x, 2+(3)
      ), 5);

    Object expr2 = "(" + gt(eq(x, (3)+2
      ), 6);

    Object expr3 =
        gt(
            qw(), eq(gt(x,
                y), 2));

vlsi avatar Feb 26 '20 09:02 vlsi