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.

Sign up for GitHub

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

Jump to bottom

[SPARK-35175][BUILD] Add linter for JavaScript source files. #32274

Closed
wants to merge 26 commits into from
Closed

[SPARK-35175][BUILD] Add linter for JavaScript source files. #32274

wants to merge 26 commits into from

Conversation

sarutak
Copy link
Member

@sarutak sarutak commented Apr 21, 2021 •
edited

What changes were proposed in this pull request?

This PR proposes to add linter for JavaScript source files.
ESLint seems to be a popular linter for JavaScript so I choose it.

Why are the changes needed?

Linter enables us to check style and keeps code clean.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manually run dev/lint-js (Node.js and npm are required).

In this PR, mainly indentation style is also fixed an linter passes.

@SparkQA
Copy link

SparkQA commented Apr 21, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42263/

@SparkQA
Copy link

SparkQA commented Apr 21, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42263/

@SparkQA
Copy link

SparkQA commented Apr 21, 2021

Test build #137736 has finished for PR 32274 at commit e2fcd9e.

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

@sarutak
Modify .rat-exclude.
990e538 
Modify lint-js.
@SparkQA
Copy link

SparkQA commented Apr 22, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42289/

@SparkQA
Copy link

SparkQA commented Apr 22, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42289/

@SparkQA
Copy link

SparkQA commented Apr 22, 2021

Test build #137762 has finished for PR 32274 at commit 990e538.

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

echohlne
echohlne reviewed Apr 23, 2021
View reviewed changes
dev/lint-js
LINT_JS_REPORT_FILE_NAME="$SPARK_ROOT_DIR/dev/lint-js-report.log"
LINT_TARGET_FILES=(
"$SPARK_ROOT_DIR/core/src/main/resources/org/apache/spark/ui/static/"
"$SPARK_ROOT_DIR/sql/core/src/main/resources/org/apache/spark/sql/execution/ui/static/"
Copy link
Contributor

@echohlne echohlne Apr 23, 2021 •
edited

Choose a reason for hiding this comment

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

I'm just curious, should the dir: $SPARK_ROOT_DIR/docs/js/" be added to the LINT_TARGET_FILES` ？

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, it seems that some files should be checked. Thanks.

@srowen
Copy link
Member

srowen commented Apr 23, 2021

Does all the current JS pass the linter?

@sarutak
Copy link
Member Author

sarutak commented Apr 24, 2021

@srowen

Does all the current JS pass the linter?

As I noted in the description, not yet.
I'd like to build a consensus on the basic lint rule in this PR, then I'll fix style based on the rule.

@SparkQA
Copy link

SparkQA commented Apr 25, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42428/

@SparkQA
Copy link

SparkQA commented Apr 25, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42428/

@SparkQA
Copy link

SparkQA commented Apr 25, 2021

Test build #137903 has finished for PR 32274 at commit d6eb61a.

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

@sarutak
Copy link
Member Author

sarutak commented May 6, 2021

@srowen I fixed style based on the current rule and now the linter passes.
Too much style changes included in this PR, so if it's better to do, I'll focus on the linter and separate style change.

@SparkQA
Copy link

SparkQA commented May 6, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42729/

@SparkQA
Copy link

SparkQA commented May 6, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42729/

@SparkQA
Copy link

SparkQA commented May 6, 2021

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42732/

srowen
srowen approved these changes May 6, 2021
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Looks OK pending tests. I think a one-time cleanup is OK if we have a linter to enforce checks. It looks mostly like indentation changes.

@SparkQA
Copy link

SparkQA commented May 6, 2021

Test build #138207 has finished for PR 32274 at commit d174384.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented May 6, 2021

Test build #138210 has finished for PR 32274 at commit 5bda9fc.

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

HyukjinKwon
HyukjinKwon approved these changes May 7, 2021
View reviewed changes
Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

I like the idea. Didn't take a close look but I think it's good enough. cc @gengliangwang too fyi

@sarutak sarutak closed this in 2634dba May 7, 2021
@sarutak
Copy link
Member Author

sarutak commented May 7, 2021

Merged to master. Thank you all.
I'll add this linter to the GA later.

gengliangwang
gengliangwang approved these changes May 7, 2021
View reviewed changes
Copy link
Member

@gengliangwang gengliangwang left a comment

Choose a reason for hiding this comment

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

Late LGTM. I just tried it locally and it works!

@sarutak sarutak mentioned this pull request May 12, 2021
Closed
HyukjinKwon pushed a commit that referenced this pull request May 12, 2021
@sarutak @HyukjinKwon
[SPARK-35377][INFRA] Add JS linter to GA
7e3446a 
### What changes were proposed in this pull request?

SPARK-35175 (#32274) added a linter for JS so let's add it to GA.

### Why are the changes needed?

To JS code keep clean.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

GA

Closes #32512 from sarutak/ga-lintjs.

Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@echohlne echohlne echohlne left review comments

@srowen srowen srowen approved these changes

@gengliangwang gengliangwang gengliangwang approved these changes

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

Assignees
No one assigned
Labels
BUILD CORE INFRA WEB UI
Projects
None yet
Milestone
No milestone
6 participants
@sarutak @SparkQA @srowen @gengliangwang @HyukjinKwon @echohlne