-
Notifications
You must be signed in to change notification settings - Fork 28.6k
[SPARK-16877][BUILD] Add rules for preventing to use Java annotations (Deprecated and Override) #14490
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.
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
Conversation
LGTM pending Jenkins. |
@@ -250,6 +250,14 @@ This file is divided into 3 sections: | |||
<customMessage>Omit braces in case clauses.</customMessage> | |||
</check> | |||
|
|||
<!-- SPARK-16877: Avoid Java annotations --> | |||
<check customId="OverrideJavaCase" level="error" class="org.scalastyle.scalariform.TokenChecker" enabled="true"> | |||
<parameters><parameter name="regex">^Override$</parameter></parameters> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to look for a leading "@" as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually tried ^@Override$
first but I found this Scala checking recognising this token as just Override
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, actually I tried RegexChecker
as well to grep this case but then I found actually I should come up with a complex regular expression for some exceptional cases such as
@Override
in comments
/**
...
* @Override
* public void close(Throwable errorOrNull) {
* // close the connection
* }
...
@Override
in codegen
...
" @Override public String toString() { return \"" + toStringValue + "\"; }}"
...
So, I had to use TokenChecker
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just reprored this locally on my machine as well (doesn't trigger with @Override
or \@Override
as the regex rule). One guess is the token checker has split them into separate tokens perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I just ran a test after cloning scalariform which scalastyle uses as lexer as below:
ScalaLexer.rawTokenise("@Override")
It seems this becomes
different tokens.
(BTW, maybe we should avoid to write @Override
as it is.. I started to feel guilty for cc him/her here and there....)
Test build #63201 has finished for PR 14490 at commit
|
retest this please |
Test build #63207 has finished for PR 14490 at commit
|
retest this please |
Test build #63215 has finished for PR 14490 at commit
|
… (Deprecated and Override) ## What changes were proposed in this pull request? This PR adds both rules for preventing to use `Deprecated` and `Override`. - Java's `Override` It seems Scala compiler just ignores this. Apparently, `override` modifier is only mandatory for " that override some other **concrete member definition** in a parent class" but not for for **incomplete member definition** (such as ones from trait or abstract), see (http://www.scala-lang.org/files/archive/spec/2.11/05-classes-and-objects.html#override) For a simple example, - Normal class - needs `override` modifier ```bash scala> class A { def say = {}} defined class A scala> class B extends A { def say = {}} <console>:8: error: overriding method say in class A of type => Unit; method say needs `override' modifier class B extends A { def say = {}} ^ ``` - Trait - does not need `override` modifier ```bash scala> trait A { def say } defined trait A scala> class B extends A { def say = {}} defined class B ``` To cut this short, this case below is possible, ```bash scala> class B extends A { | Override | def say = {} | } defined class B ``` we can write `Override` annotation (meaning nothing) which might confuse engineers that Java's annotation is working fine. It might be great if we prevent those potential confusion. - Java's `Deprecated` When `Deprecated` is used, it seems Scala compiler recognises this correctly but it seems we use Scala one `deprecated` across codebase. ## How was this patch tested? Manually tested, by inserting both `Override` and `Deprecated`. This will shows the error messages as below: ```bash Scalastyle checks failed at following occurrences: [error] ... : deprecated should be used instead of java.lang.Deprecated. ``` ```basg Scalastyle checks failed at following occurrences: [error] ... : override modifier should be used instead of java.lang.Override. ``` Author: hyukjinkwon <gurwls223@gmail.com> Closes #14490 from HyukjinKwon/SPARK-16877. (cherry picked from commit 1d78157) Signed-off-by: Sean Owen <sowen@cloudera.com>
Merged to master, 2.0 |
What changes were proposed in this pull request?
This PR adds both rules for preventing to use
@Deprecated
and@Override
.Java's
@Override
It seems Scala compiler just ignores this. Apparently,
override
modifier is only mandatory for " that override some other concrete member definition in a parent class" but not for for incomplete member definition (such as ones from trait or abstract), see (http://www.scala-lang.org/files/archive/spec/2.11/05-classes-and-objects.html#override)For a simple example,
override
modifieroverride
modifierTo cut this short, this case below is possible,
we can write
@Override
annotation (meaning nothing) which might confuse engineers that Java's annotation is working fine. It might be great if we prevent those potential confusion.Java's
@Deprecated
When
@Deprecated
is used, it seems Scala compiler recognises this correctly but it seems we use Scala one@deprecated
across codebase.How was this patch tested?
Manually tested, by inserting both
@Override
and@Deprecated
. This will shows the error messages as below:Scalastyle checks failed at following occurrences: [error] ... : @deprecated should be used instead of @java.lang.Deprecated.