mccheah edited a comment on issue #28: Apply baseline checkstyle for iceberg-api only URL: https://github.com/apache/incubator-iceberg/pull/28#issuecomment-445041145 @rdblue thanks for the review! > Static imports should come after non-static imports. Per [Google's style guide](https://google.github.io/styleguide/javaguide.html#s3.3.3-import-ordering-and-spacing) (actually much of Baseline inherits the Google style guide). Edit: Think it's fine if we want to be opinionated about moving static imports to be after non-static if we want to minimize code churn. In the long term it would be good to think about changing the import order to be consistent with the Google style guide. > * I like using static imports to cut down on code in symbol references, like EQ instead of Expressions.Operators.EQ. Fair enough, let's add exclusions as we see fit. I already added some exclusion, I believe for literals and operators. We can add them for expressions as well. I don't think we want to allow static imports globally though, it leads to difficulty to trace a method call back to its source without an IDE (i.e. in CR). > * Non-static imports should be a single group in alphabetical order. Not sure why some moved. That's what checkstyle should be supporting with this patch though? There were some that moved the core JDK imports into the main import block. I'll double check. > * I definitely prefer UUIDLiteral to UuidLiteral. That's also public API so we should avoid changing it. Fair enough. Baseline wants to enforce camelCase for everything, so names with consecutive capital letters are not allowed. See [here](https://google.github.io/styleguide/javaguide.html#s5.2.8-type-variable-names) and [here](https://google.github.io/styleguide/javaguide.html#s5.3-camel-case). We can add an exclusion for `ID` and `UUID`, among others. > * 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`. Baseline's checkstyle requires all variables to not hide fields. This meant that for `fieldIdToFind`, the parameter `fieldId` was hiding the field `fieldId` of the same class. All these variable name changes were to deconflict. Can we define parameter and field names that don't conflict but that are sensible on a case by case basis? I made an initial attempt here, can revise these again. > * Seems like it would be easier to allow or require javadoc sentences to end in '.' It's for the `@return` blocks. Opinion from baseline but we can revert it. > * Like allowing '.', using operators at the end of a line than a the start of the next would require fewer changes. I think we want to only enforce one or the other; we can find how to make the regex enforce the operators to be on the same line rather than on the next. Or we can just start making all operators move to the next line, as we do [here](https://google.github.io/styleguide/javaguide.html#s4.5.1-line-wrapping-where-to-break). > Does this enforce a line length of 100 characters? 120 characters by default. > Why does this require so many Palantir plugins? Are those required? These are all Palantir's open-source tooling! Granted one can get all of them automatically applied by just applying the `com.palantir.baseline` plugin. The ideal end state is to apply `com.palantir.baseline` globally, but doing that requires not only fixing checkstyle everywhere but also applying error-prone and other linting plugins that require further patches. For example I noticed error-prone complaining about some of the ways we write `Preconditions` checks.
---------------------------------------------------------------- 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
