Add checkstyle in maven build #2610
Conversation
|
build |
There was a problem hiding this comment.
This PR needs to run checkstyle on the entire eligible codebase and update files accordingly to match the desired style. Otherwise we're going to have a parade of PRs where the change they want to make is relatively small, but the change is dwarfed by the style fixes required to pass CI. This would also help sanity check that the configured style checks are doing what we desire in this PR.
dev/checkstyle.sh
Outdated
| set -ex | ||
|
|
||
| # Assuming you are in the root of your git repository | ||
| MODIFIED_FILES=$(git diff --name-only "origin/${GITHUB_BASE_REF}") |
There was a problem hiding this comment.
agree with Jason, We would like to not only check style for incremental changes.
and this script is current for github action only with the ENV provided by github.
can we just check against all eligible code with the merged commit for PR? and developers could also run the script to check their change locally before filing a PR
and in that case we do not need below in action def then
with:
submodules: true
fetch-depth: 0
- name: Fetch branches and tags
run: git fetch --all
There was a problem hiding this comment.
See comments below.
I ran the checkstyle which detected 744 errors, and most of them not easy to fix since it's not simple formatting issue, for example, missing docs, naming abbreviations, etc. The effort to fix them all is quite huge. |
Then this PR should not go in like this. It adds a significant burden to all future PRs, because if some PR needs to touch many files, now the burden you didn't want to perform is forced on an unrelated PR. Like I said, we do not want any required fixes to be performed "on the fly" via unrelated PRs. It's mixing purposes and makes those PRs harder to write and harder to read. Can we turn on only some of the checkstyle rules for this first version and update any errant files accordingly? We can then turn on more and more checks via foilowup PRs to make the changes manageable. |
Can we try disabling some rules (e.g., docs) initially and fixing those critical ones only within this change? |
|
build |
|
build |
|
build |
| * SQL is: | ||
| * select | ||
| * case | ||
| * when bool_1_expr then "value_1" | ||
| * when bool_2_expr then "value_2" | ||
| * when bool_3_expr then "value_3" | ||
| * else "value_else" | ||
| * end | ||
| * from tab |
There was a problem hiding this comment.
The original way the comment was written was easier to read. IMO style checks should not be messing with leading whitespace within a comment, as it's likely intentional. This comment applies to other places where leading whitespace of indented blocks was removed.
| * Since the result column may or may not be used regardless of the value of hasFailure, we | ||
| * need to keep it and let the caller to decide. | ||
| */ | ||
| public static class CastFloatToDecimalResult { |
There was a problem hiding this comment.
This is a huge PR, and a large reason for that is the enforcement of a strict declaration order of inner classes and methods. I think that should be deferred to a followup since I'm not sure everyone on the team would agree the new order is better.
Once that is removed, this PR should be relatively small once whitespace diffs are ignored.
Close #2591.
The style check only checks modified files in each pr.