Skip to content

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Dec 9, 2019

What changes were proposed in this pull request?

This PR aims to prevent Java checkstyle violation.

Why are the changes needed?

Historically, we disabled this because there was no way to check this in PR Builder (9b98aef).
However, now we have GitHub Action which do this every PR.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manually.

@dongjoon-hyun dongjoon-hyun changed the title Enable checkstyle [SPARK-30194][BUILD] Re-enable checkstyle for Java Dec 9, 2019
@dongjoon-hyun dongjoon-hyun requested a review from srowen December 9, 2019 17:19
@srowen
Copy link
Member

srowen commented Dec 9, 2019

Sure, but, heh does it actually pass right now?

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Dec 9, 2019

Yes. lint-java failure was fixed at the last commit. The following job also was recovered together.

@dongjoon-hyun
Copy link
Member Author

Oh, let me check the build-error part once more.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-30194][BUILD] Re-enable checkstyle for Java [WIP][SPARK-30194][BUILD] Re-enable checkstyle for Java Dec 9, 2019
@SparkQA
Copy link

SparkQA commented Dec 9, 2019

Test build #115047 has finished for PR 26820 at commit 4b8d0f1.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun dongjoon-hyun changed the title [WIP][SPARK-30194][BUILD] Re-enable checkstyle for Java [WIP][SPARK-30194][BUILD][test-maven] Re-enable checkstyle for Java Dec 9, 2019
@dongjoon-hyun
Copy link
Member Author

Retest this please.

@HeartSaVioR
Copy link
Contributor

Btw, I'm feeling that it might be too strict to throw error on unused imports; as it could happen when multiple PRs modify the same file. Scala checkstyle doesn't throw error on unused imports, right?

@dongjoon-hyun
Copy link
Member Author

Ya. That's a good point. Let me think about that, too.

@HyukjinKwon
Copy link
Member

We enabled checkstyle in PR builder via #21399. So does this PR only apply when PR has the title [test-maven]?

<version>3.1.0</version>
<configuration>
<failOnViolation>false</failOnViolation>
<failOnViolation>true</failOnViolation>
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we can just completely switch linter-java to sbt-checkstyle completely so that we don't suffer from the differences anymore.

I have seen some differences so far, e.g. #26818 and #21801

@SparkQA
Copy link

SparkQA commented Dec 10, 2019

Test build #115050 has finished for PR 26820 at commit 4b8d0f1.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member Author

Hi, All. Thank you for review.
Originally, I made this PR because one Jenkins job died because of lint-java failure.
However, I'll drop this approach since this is more risky than I thought.

We had better remove the lint-java from that Jenkins job instead because GitHub Action is cheap and finished faster than Jenkins jobs.

@dongjoon-hyun dongjoon-hyun deleted the enable-lint-java branch December 10, 2019 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants