Skip to content

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

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

Closed
wants to merge 1 commit into from
Closed

[SPARK-16877][BUILD] Add rules for preventing to use Java annotations (Deprecated and Override) #14490

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions scalastyle-config.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

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?

Copy link
Member Author

@HyukjinKwon HyukjinKwon Aug 4, 2016 •
edited
Loading

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.

Copy link
Member

Choose a reason for hiding this comment

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

Hm so this matches "@OverRide" but not "Override" now, but would reverse if the regex included "@"? That sounds flipped. @ isn't a special char. You're doubly sure that's right? It also seems like this is matching "Override" on a line alone but should be looking for "@OverRide" anywhere.

Copy link
Member Author

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.

Copy link
Contributor

@holdenk holdenk Aug 4, 2016 •
edited
Loading

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?

Copy link
Member Author

@HyukjinKwon HyukjinKwon Aug 4, 2016 •
edited
Loading

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

2016-08-04 1 29 11

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

<customMessage>override modifier should be used instead of @java.lang.Override.</customMessage>
</check>

<check level="error" class="org.scalastyle.scalariform.DeprecatedJavaChecker" enabled="true"></check>

<!-- ================================================================================ -->
<!-- rules we'd like to enforce, but haven't cleaned up the codebase yet -->
<!-- ================================================================================ -->
Expand Down