NVIDIA / spark-rapids-jni

RAPIDS Accelerator JNI For Apache Spark
Apache License 2.0
41 stars 66 forks source link

Add checkstyle in maven build #2610

Open liurenjie1024 opened 6 days ago

liurenjie1024 commented 6 days ago

Close #2591.

The style check only checks modified files in each pr.

liurenjie1024 commented 6 days ago

build

liurenjie1024 commented 5 days ago

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.

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.

jlowe commented 5 days ago

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.

pxLi commented 5 days ago

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.

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.

Can we try disabling some rules (e.g., docs) initially and fixing those critical ones only within this change?

liurenjie1024 commented 5 days ago

I have auto fixed formats, and disabled not import rules, PTAL cc @jlowe @pxLi

liurenjie1024 commented 5 days ago

build

liurenjie1024 commented 5 days ago

build

liurenjie1024 commented 5 days ago

build