[SPARK-21259] More rules for scalastyle #18471
Conversation
gengliangwang
force-pushed
the
scalastyle
branch
from
f68e600 to
d1b4bbe
Compare
|
Most changes are done by my ruby script: lines = `./dev/lint-scala 2>&1 | grep "before\\|after"`.split("\n")
lines.each do |line|
file, line_num=line.split(/[ ,:]/)[1,2]
if line =~ /Space before token :/
`sed -i '' '#{line_num}s/ :/:/g' #{file}`
elsif line =~ /Space before token \)/
`sed -i '' '#{line_num}s/ )/)/g' #{file}`
elsif line =~ /Space after token \(/
`sed -i '' '#{line_num}s/( /(/g' #{file}`
end
end |
| @@ -68,7 +68,9 @@ class Accumulable[R, T] private ( | |||
| } | |||
|
|
|||
| private[spark] def this(initialValue: R, param: AccumulableParam[R, T], name: Option[String]) = { | |||
| // scalastyle:off | |||
| this(initialValue, param, name, false /* countFailedValues */) | |||
There was a problem hiding this comment.
nit: countFailedValues = false is better, this style is only useful for java
scalastyle-config.xml
Outdated
| @@ -46,7 +46,7 @@ This file is divided into 3 sections: | |||
|
|
|||
| <check level="error" class="org.scalastyle.file.FileTabChecker" enabled="true"></check> | |||
|
|
|||
| <check level="error" class="org.scalastyle.file.HeaderMatchesChecker" enabled="true"> | |||
| <check level="error" class="org.scalastyle.file.HeaderMatchesChecker" enabled="false"> | |||
There was a problem hiding this comment.
Just a question. Should we disable this rule to enable the new rule?
There was a problem hiding this comment.
No, I made a mistake.
scalastyle-config.xml
Outdated
| </parameters> | ||
| </check> | ||
|
|
||
| <check level="error" class="org.scalastyle.scalariform.DisallowSpaceAfterTokenChecker" enabled="true"> |
There was a problem hiding this comment.
Another question. Could we add customId and disable this only when scalastyle:off and scalastyle:on is required?
There was a problem hiding this comment.
Thanks, I have updated the xml file.
|
Test build #78937 has finished for PR 18471 at commit
|
|
Test build #78940 has finished for PR 18471 at commit
|
|
Test build #78948 has finished for PR 18471 at commit
|
|
Test build #78951 has finished for PR 18471 at commit
|
|
I tend to agree with the style change, but, am concerned about what a huge diff this creates for little gain. I personally am not sure I'd bother with the colon change; some instances of space before colon are conventional like The space inside parens change seems OK, maybe smaller. |
| @@ -52,8 +52,8 @@ private[spark] class BroadcastManager( | |||
|
|
|||
| private val nextBroadcastId = new AtomicLong(0) | |||
|
|
|||
| def newBroadcast[T: ClassTag](value_ : T, isLocal: Boolean): Broadcast[T] = { | |||
| broadcastFactory.newBroadcast[T](value_, isLocal, nextBroadcastId.getAndIncrement()) | |||
| def newBroadcast[T: ClassTag](_value: T, isLocal: Boolean): Broadcast[T] = { | |||
There was a problem hiding this comment.
I am not sure of this naming change. I mean the change does not look particulaily better to me. I believe we have a open PR - databricks/scala-style-guide#52 which appearntly stays against some cases here.
There was a problem hiding this comment.
I think it's not about adding _ to the name of private val, but kind of avoiding name conflict if 2 variables have similar meaning. At least this is fine to me, I also do this a lot...
There was a problem hiding this comment.
I am fine if we prefer a specific way. My only worry is we happen to fix it back again in the future but no one stays against because they don't know which way is preferred...
There was a problem hiding this comment.
Maybe, this can be an example other contributors like me can refer in the future BTW.
| this(initialValue, param, name, false /* countFailedValues */) | ||
| // scalastyle:on |
There was a problem hiding this comment.
I believe we could enable and disable via scalastyle:on customid rather than switching off whole style rules here IIRC.
There was a problem hiding this comment.
Oh, nvm. I just read your commit log ...
|
My only concern is |
|
Do we still want this? |
|
Close this for now. |
What changes were proposed in this pull request?
Add more rules for scalastyle
During code review, we spent so much time on code style issues.
It would be great if we add rules:
How was this patch tested?
unit tests, integration tests
Please review http://spark.apache.org/contributing.html before opening a pull request.