Skip to content

Conversation

@HeartSaVioR
Copy link
Contributor

@HeartSaVioR HeartSaVioR commented Dec 9, 2019

What changes were proposed in this pull request?

This patch fixes the Java code style violations in SPARK-30159 (#26788) which are caught by lint-java (Github Action caught it and I can reproduce it locally). Looks like Jenkins build may have different policy on checking Java style check or less accurate.

Why are the changes needed?

Java linter starts complaining.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

lint-java passed locally

This closes #26819

@dongjoon-hyun
Copy link
Member

Thank you, @HeartSaVioR !

Copy link
Member

@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.

+1, LGTM. This passed GitHub Action linter.
Merged to master.

@gengliangwang
Copy link
Member

@HeartSaVioR Thanks for the follow-up

@SparkQA
Copy link

SparkQA commented Dec 9, 2019

Test build #115044 has finished for PR 26818 at commit 84d6466.

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

@HeartSaVioR
Copy link
Contributor Author

Thanks all for reviewing and merging!

@HeartSaVioR HeartSaVioR deleted the SPARK-30159-FOLLOWUP branch December 9, 2019 20:50
@HyukjinKwon
Copy link
Member

Thanks for fixing it. Hm, I wonder why there's a diff ...

@HeartSaVioR
Copy link
Contributor Author

Hm, I wonder why there's a diff ...

#26820 seems to be an answer; we don't fail the build even though there're style guide violations. In script of Github Action, we run lint-java which fails the build.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Dec 10, 2019

Yes.

@HyukjinKwon
Copy link
Member

I mean, Jenkins uses sbt-checkstyle via SBT to reuse previously compiled results. Github Action usese Maven linter. Seems there's a difference between them.

I don't believe the linter doesn't fail in PR builder if this isn't a regression - I tested it when I added it.

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