rdblue commented on issue #28: Apply baseline checkstyle for iceberg-api only URL: https://github.com/apache/incubator-iceberg/pull/28#issuecomment-445036100 I think most of the changes are fine, but I noticed a few things: * Static imports should come after non-static imports. * I like using static imports to cut down on code in symbol references, like EQ instead of Expressions.Operators.EQ. * Non-static imports should be a single group in alphabetical order. Not sure why some moved. * I definitely prefer UUIDLiteral to UuidLiteral. That's also public API so we should avoid changing it. * Some short variable name changes are fine (o -> other) but the L and W variables used in Transforms are from the spec, so I'd rather add an exclusion for them. * Some variable name changes lose information, like the use of `classes` instead of `javaClasses`. That was intentional because the class is the one used for the Java in-memory representation. * Is `fieldIdToFind` really better than `fieldId`? What rule is this matching? Same with `newStruct` instead of `struct`. * Seems like it would be easier to allow or require javadoc sentences to end in '.' * Like allowing '.', using operators at the end of a line than a the start of the next would require fewer changes. I like the updates for: * Removing public modifiers when the class is package-private or private * Requiring a private constructor for classes with only static helpers Does this enforce a line length of 100 characters? Why does this require so many Palantir plugins? Are those required?
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
