Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ORC-1486: Fixes checkstyle violation in test classes for orc-core module #1600

Closed
wants to merge 1 commit into from

Conversation

dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Aug 26, 2023

What changes were proposed in this pull request?

This PR fixes checkstyle violation in test sources in orc-core module.

Indentation rule is suppressed in some files explicitly.

Why are the changes needed?

The change shall ensure our test code adhere's to same formatting as our source

How was this patch tested?

Remove the following and run Checkstyle on core module.

<suppress checks="Indentation" files="src/test/*"/>
<suppress checks="RedundantModifier" files="src/test/*"/>
<suppress checks="ModifierOrder" files="src/test/*"/>
<suppress checks="UpperEll" files="src/test/*"/>
<suppress checks="NeedBraces" files="src/test/*"/>
<suppress checks="RegexpSingleline" files="src/test/*"/>

$ cd java

$ git diff
diff --git a/java/checkstyle-suppressions.xml b/java/checkstyle-suppressions.xml
index 32e04a588..0256e3b77 100644
--- a/java/checkstyle-suppressions.xml
+++ b/java/checkstyle-suppressions.xml
@@ -44,10 +44,4 @@
   <suppress checks="CustomImportOrder" files="src/test/*"/>

   <!-- Remove the following rule when the test code clean up is completed. -->
-  <suppress checks="Indentation" files="src/test/*"/>
-  <suppress checks="RedundantModifier" files="src/test/*"/>
-  <suppress checks="ModifierOrder" files="src/test/*"/>
-  <suppress checks="UpperEll" files="src/test/*"/>
-  <suppress checks="NeedBraces" files="src/test/*"/>
-  <suppress checks="RegexpSingleline" files="src/test/*"/>
 </suppressions>

$ mvn checkstyle:check --pl core
Running `/Users/dongjoon/APACHE/orc-merge/java/mvnw`...
Using `mvn` from path: /opt/homebrew/bin/mvn
[INFO] Scanning for projects...
[INFO]
[INFO] ----------------------< org.apache.orc:orc-core >-----------------------
[INFO] Building ORC Core 2.0.0-SNAPSHOT
[INFO]   from pom.xml
[INFO] --------------------------------[ jar ]---------------------------------
[INFO]
[INFO] --- checkstyle:3.3.0:check (default-cli) @ orc-core ---
[INFO] You have 0 Checkstyle violations.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  4.228 s
[INFO] Finished at: 2023-08-25T22:13:02-07:00
[INFO] ------------------------------------------------------------------------

This closes #1590

@github-actions github-actions bot added the JAVA label Aug 26, 2023
@dongjoon-hyun
Copy link
Member Author

cc @mystic-lama

@dongjoon-hyun dongjoon-hyun changed the title ORC-1486: Fixes checkstyle violation in test classes for orc-core module ORC-1486: Fixes checkstyle violation in test classes for orc-core module Aug 26, 2023
Copy link
Member Author

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope we can proceed in this way module by module, @mystic-lama .

@paliwalashish
Copy link
Contributor

@dongjoon-hyun Thank you so much. I only get time during weekend or evening to work on OSS. Sadly got pulled into work, which took that time. I shall try to close things faster. Let me catch up with things and we can get this done ASAP

-9181829309989854913l};
long[] inp = new long[]{4513343538618202719L, 4513343538618202711L,
2911390882471569739L,
-9181829309989854913L};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we have this block like this? I find it more easy on my eyes. We don't have to fix these things now. I can take care of this later

long[] inp = new long[]{4513343538618202719L, 4513343538618202711L,
                        2911390882471569739L, -9181829309989854913L};                                                                               

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I asked which rule complains about that, @mystic-lama ?

As you see, this PR aims to introduce the minimal changes which means to touch only Checkstyle really complains. The verification is described in the PR description, too.

To simply put, if we mix different themes arbitrarily like that, it tends to make a mistake because of the human error factor. It makes reviewers difficulty to review the PRs. It ends up as a situation we cannot merge it.

Of course, we can take care of it later independently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your point. I don't recall the rule, and did not mean to fix it in this PR
This is more for future, when we make code changes. I shall keep an eye for this and next time it hits, may be we can refine the rule.

Copy link
Contributor

@paliwalashish paliwalashish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 LGTM

Thanks a lot @dongjoon-hyun

@dongjoon-hyun
Copy link
Member Author

BTW, please note that Apache ORC has 3 year maintenance policy. It means ORC committers need to maintain branch-1.9 and branch-1.8 and branch-1.7 for three years and usually it requires us to backport from main to branch-1.9 to branch-1.8 and branch-1.7 sequentially if needed. So, we are unable to accept a drastic change like your previous PR even for one time activity. It will tease us for 3 years.

@dongjoon-hyun dongjoon-hyun added this to the 2.0.0 milestone Aug 27, 2023
@dongjoon-hyun
Copy link
Member Author

Merged to main for Apache ORC 2.0.

$ git log -n1 | tail -n2
    Authored-by: mystic-lama <mysticlama000@gmail.com>
    Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

@dongjoon-hyun dongjoon-hyun deleted the ORC-1486 branch August 27, 2023 02:36
@dongjoon-hyun
Copy link
Member Author

Let me make a PR for the rest of module. It seems that we can close this checkstyle task today.

@dongjoon-hyun
Copy link
Member Author

Here is the PR.
#1601

@paliwalashish
Copy link
Contributor

BTW, please note that Apache ORC has 3 year maintenance policy. It means ORC committers need to maintain branch-1.9 and branch-1.8 and branch-1.7 for three years and usually it requires us to backport from main to branch-1.9 to branch-1.8 and branch-1.7 sequentially if needed. So, we are unable to accept a drastic change like your previous PR even for one time activity. It will tease us for 3 years.

@dongjoon-hyun This is good to know.

cxzl25 pushed a commit to cxzl25/orc that referenced this pull request Jan 11, 2024
…odule

### What changes were proposed in this pull request?

This PR fixes checkstyle violation in test sources in orc-core module.

`Indentation` rule is suppressed in some files explicitly.

### Why are the changes needed?

The change shall ensure our test code adhere's to same formatting as our source

### How was this patch tested?

Remove the following and run Checkstyle on `core` module.

https://github.com/apache/orc/blob/7787669c444d0cf18ba91effbba34b5608370b5b/java/checkstyle-suppressions.xml#L31-L36

```
$ cd java

$ git diff
diff --git a/java/checkstyle-suppressions.xml b/java/checkstyle-suppressions.xml
index 32e04a588..0256e3b77 100644
--- a/java/checkstyle-suppressions.xml
+++ b/java