Skip to content

Conversation

@arucard21
Copy link
Contributor

@arucard21 arucard21 commented Mar 22, 2018

What changes were proposed in this pull request?

We re-enabled the Scalastyle checker on a line of code. It was previously disabled, but it does not violate any of the rules. So there's no reason to disable the Scalastyle checker here.

How was this patch tested?

We tested this by running build/mvn scalastyle:check after removing the comments that disable the checker. This check passed with no errors or warnings for Spark Core

[INFO]                                                                         
[INFO] ------------------------------------------------------------------------
[INFO] Building Spark Project Core 2.4.0-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO] 
[INFO] --- scalastyle-maven-plugin:1.0.0:check (default-cli) @ spark-core_2.11 ---
Saving to outputFile=<path to local dir>/spark/core/target/scalastyle-output.xml
Processed 485 file(s)
Found 0 errors
Found 0 warnings
Found 0 infos

We did not run all tests (with dev/run-tests) since this Scalastyle check seemed sufficient.

Co-contributors:

@chialun-yeh
@Hrayo712
@vpourquie

@arucard21 arucard21 changed the title Remove comments that unnecessarily disable Scalastyle check [SPARK-23769][Core]Remove comments that unnecessarily disable Scalastyle check Mar 22, 2018
@HyukjinKwon
Copy link
Member

Minor change does't need a JIRA though. Mind checking if there are similar instances while we are here?

@HyukjinKwon
Copy link
Member

Also, I believe more correct way is ./dev/lint-scala

@arucard21
Copy link
Contributor Author

arucard21 commented Mar 22, 2018

@HyukjinKwon I just ran ./dev/lint-scala as well and the result is Scalastyle checks passed.

As I mentioned in the JIRA issue, we looked at all the instances of scalastyle:off comments in spark/core/src/main/scala (so only the production code of the Core component of Spark). This is the only one where we noticed that disabling the Scalastyle checker was entirely unnecessary.

But we found another instance where it would be very easy to fix (even for beginners like us), so I'll create a PR for that as well. Since you said a minor change doesn't need a JIRA, I'll create the pull request only.

@HyukjinKwon
Copy link
Member

That's fine. Let's invlove another insrance here.

@HyukjinKwon
Copy link
Member

ok to teat

@arucard21
Copy link
Contributor Author

@HyukjinKwon I had actually already created a separate branch for the other fix and was already creating a separate PR for it. So the other fix is in #20882.

I also think that the typo in your previous comment may have caused Jenkins to miss the keyword(s) to correctly detect it.

@HyukjinKwon
Copy link
Member

ok to test

@HyukjinKwon
Copy link
Member

let's include that change here.

@arucard21
Copy link
Contributor Author

let's include that change here.

OK, I'll push the change in a few minutes. Should I close the other PR then?

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Mar 22, 2018

yea please

@arucard21
Copy link
Contributor Author

arucard21 commented Mar 22, 2018

Just pushed another fix for Scalastyle code checking. It just replaces a URL with a shortened version so we don't violate the max line length. Also tested with ./dev/lint-scala and closed the other PR.

@HyukjinKwon Thanks for your help with this. I really appreciate it.

@SparkQA
Copy link

SparkQA commented Mar 22, 2018

Test build #88526 has finished for PR 20880 at commit de5c33a.

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

@SparkQA
Copy link

SparkQA commented Mar 22, 2018

Test build #88525 has finished for PR 20880 at commit b6088e2.

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

Copy link
Contributor

@jiangxb1987 jiangxb1987 left a comment

Choose a reason for hiding this comment

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

LGTM

asfgit pushed a commit that referenced this pull request Mar 23, 2018
…tyle check

## What changes were proposed in this pull request?

We re-enabled the Scalastyle checker on a line of code. It was previously disabled, but it does not violate any of the rules. So there's no reason to disable the Scalastyle checker here.

## How was this patch tested?

We tested this by running `build/mvn scalastyle:check` after removing the comments that disable the checker. This check passed with no errors or warnings for Spark Core
```
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] Building Spark Project Core 2.4.0-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO]
[INFO] --- scalastyle-maven-plugin:1.0.0:check (default-cli)  spark-core_2.11 ---
Saving to outputFile=<path to local dir>/spark/core/target/scalastyle-output.xml
Processed 485 file(s)
Found 0 errors
Found 0 warnings
Found 0 infos
```
We did not run all tests (with `dev/run-tests`) since this Scalastyle check seemed sufficient.

## Co-contributors:
chialun-yeh
Hrayo712
vpourquie

Author: arucard21 <[email protected]>

Closes #20880 from arucard21/scalastyle_util.

(cherry picked from commit 6ac4fba)
Signed-off-by: hyukjinkwon <[email protected]>
@HyukjinKwon
Copy link
Member

Merged to master and branch-2.3.

@asfgit asfgit closed this in 6ac4fba Mar 23, 2018
@arucard21 arucard21 deleted the scalastyle_util branch March 23, 2018 15:22
peter-toth pushed a commit to peter-toth/spark that referenced this pull request Oct 6, 2018
…tyle check

## What changes were proposed in this pull request?

We re-enabled the Scalastyle checker on a line of code. It was previously disabled, but it does not violate any of the rules. So there's no reason to disable the Scalastyle checker here.

## How was this patch tested?

We tested this by running `build/mvn scalastyle:check` after removing the comments that disable the checker. This check passed with no errors or warnings for Spark Core
```
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] Building Spark Project Core 2.4.0-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO]
[INFO] --- scalastyle-maven-plugin:1.0.0:check (default-cli)  spark-core_2.11 ---
Saving to outputFile=<path to local dir>/spark/core/target/scalastyle-output.xml
Processed 485 file(s)
Found 0 errors
Found 0 warnings
Found 0 infos
```
We did not run all tests (with `dev/run-tests`) since this Scalastyle check seemed sufficient.

## Co-contributors:
chialun-yeh
Hrayo712
vpourquie

Author: arucard21 <[email protected]>

Closes apache#20880 from arucard21/scalastyle_util.

(cherry picked from commit 6ac4fba)
Signed-off-by: hyukjinkwon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants