Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(699)

Issue 29347926: Issue 4248 - Add codestyle check

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 months, 1 week ago by anton
Modified:
13 hours, 37 minutes ago
Visibility:
Public.

Description

Issue 4248 - Add codestyle check

Patch Set 1 #

Total comments: 36

Patch Set 2 : commented out (not removed for now) inline conditional check #

Patch Set 3 : changed import order - `org.adblockplus` package first #

Total comments: 2

Patch Set 4 : added Gradle support, removed Ant support, added original files URLs #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+917 lines, -82 lines) Patch
M build.gradle View 1 2 3 1 chunk +7 lines, -0 lines 2 comments Download
M libadblockplus-android-settings/build.gradle View 1 2 3 2 chunks +22 lines, -2 lines 1 comment Download
M libadblockplus-android-tests/build.gradle View 1 2 3 1 chunk +30 lines, -30 lines 0 comments Download
M libadblockplus-android-webview/build.gradle View 1 2 3 1 chunk +42 lines, -20 lines 0 comments Download
M libadblockplus-android-webviewapp/build.gradle View 1 2 3 2 chunks +22 lines, -1 line 0 comments Download
M libadblockplus-android/build.gradle View 1 2 3 1 chunk +43 lines, -21 lines 0 comments Download
A + third_party/checkstyle/java/res/header.txt View 1 2 3 1 chunk +1 line, -8 lines 1 comment Download
A third_party/checkstyle/java/rules/eyeo_checks.xml View 1 2 3 1 chunk +185 lines, -0 lines 2 comments Download
A third_party/checkstyle/java/rules/google_checks.xml View 1 2 3 1 chunk +213 lines, -0 lines 0 comments Download
A third_party/checkstyle/java/rules/mozilla_checks.xml View 1 2 3 1 chunk +172 lines, -0 lines 0 comments Download
A third_party/checkstyle/java/rules/sun_checks.xml View 1 2 3 1 chunk +180 lines, -0 lines 0 comments Download

Messages

Total messages: 13
anton
header.txt is file header that every java file should have. (for some reason Rietveld shows ...
9 months, 1 week ago (2016-07-18 12:57:33 UTC) #1
anton
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml#newcode67 third_party/checkstyle/java/rules/eyeo_checks.xml:67: <module name="Header"> added this rule (pass filename as argument) ...
9 months, 1 week ago (2016-07-19 06:45:45 UTC) #2
anton
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml#newcode110 third_party/checkstyle/java/rules/eyeo_checks.xml:110: <property name="max" value="100"/> according to mozilla's style one could ...
9 months, 1 week ago (2016-07-21 10:12:48 UTC) #3
diegocarloslima
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml#newcode46 third_party/checkstyle/java/rules/eyeo_checks.xml:46: <module name="NewlineAtEndOfFile"/> I think that it would be good ...
5 months, 3 weeks ago (2016-11-03 14:00:51 UTC) #4
anton
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml#newcode46 third_party/checkstyle/java/rules/eyeo_checks.xml:46: <module name="NewlineAtEndOfFile"/> On 2016/11/03 14:00:51, diegocarloslima wrote: > I ...
5 months, 3 weeks ago (2016-11-07 07:40:45 UTC) #5
René Jeschke
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml#newcode125 third_party/checkstyle/java/rules/eyeo_checks.xml:125: <module name="WhitespaceAfter"/> On 2016/11/03 14:00:50, diegocarloslima wrote: > On ...
5 months, 1 week ago (2016-11-21 11:44:20 UTC) #6
anton
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml#newcode130 third_party/checkstyle/java/rules/eyeo_checks.xml:130: <module name="ModifierOrder"/> On 2016/11/21 11:44:19, René Jeschke wrote: > ...
5 months ago (2016-11-23 12:01:12 UTC) #7
René Jeschke
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml#newcode103 third_party/checkstyle/java/rules/eyeo_checks.xml:103: <property name="groups" value="org.mozilla,com,net,org,android,java,javax"/> On 2016/11/03 14:00:51, diegocarloslima wrote: > ...
5 months ago (2016-11-24 14:28:18 UTC) #8
anton
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkstyle/java/rules/eyeo_checks.xml#newcode103 third_party/checkstyle/java/rules/eyeo_checks.xml:103: <property name="groups" value="org.mozilla,com,net,org,android,java,javax"/> On 2016/11/24 14:28:18, René Jeschke wrote: ...
4 months, 3 weeks ago (2016-12-02 06:21:09 UTC) #9
diegocarloslima
https://codereview.adblockplus.org/29347926/diff/29366728/third_party/checkstyle/java/rules/google_checks.xml File third_party/checkstyle/java/rules/google_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29366728/third_party/checkstyle/java/rules/google_checks.xml#newcode6 third_party/checkstyle/java/rules/google_checks.xml:6: <!-- Maybe would be good the put the link ...
2 weeks, 6 days ago (2017-04-07 13:46:38 UTC) #10
anton
https://codereview.adblockplus.org/29347926/diff/29412565/build.gradle File build.gradle (right): https://codereview.adblockplus.org/29347926/diff/29412565/build.gradle#newcode21 build.gradle:21: } my original intention was to describe `checkstyle` params ...
1 week, 6 days ago (2017-04-14 12:02:28 UTC) #11
anton
On 2017/04/07 13:46:38, diegocarloslima wrote: > https://codereview.adblockplus.org/29347926/diff/29366728/third_party/checkstyle/java/rules/google_checks.xml > File third_party/checkstyle/java/rules/google_checks.xml (right): > > https://codereview.adblockplus.org/29347926/diff/29366728/third_party/checkstyle/java/rules/google_checks.xml#newcode6 > ...
1 week, 6 days ago (2017-04-14 12:02:52 UTC) #12
diegocarloslima
13 hours, 37 minutes ago (2017-04-27 12:58:45 UTC) #13
https://codereview.adblockplus.org/29347926/diff/29412565/build.gradle
File build.gradle (right):

https://codereview.adblockplus.org/29347926/diff/29412565/build.gradle#newcode21
build.gradle:21: }
On 2017/04/14 12:02:28, anton wrote:
> my original intention was to describe `checkstyle` params here to have single
> description. But this behaviour depends on gradle version and i did not found
a
> way to have the same description for all the version. So just copy-pasted it
to
> projects `build.gradle`. BTW i did not add checkstyle for `tests` module,
should
> i?

Can't you define the checkstyle version here (or in settings.gradle) by
declaring "project.ext.checkstyleToolVersion=6.19" and having it used by each
project by reading rootProject.checkstyleToolVersion? If that doesn't works
because of different Gradle versions, have you also considered using a
.properties file where you would declare the checkstyle version?

Yeah, I think it'll be good to check style for the tests module as well

https://codereview.adblockplus.org/29347926/diff/29412565/libadblockplus-andr...
File libadblockplus-android-settings/build.gradle (right):

https://codereview.adblockplus.org/29347926/diff/29412565/libadblockplus-andr...
libadblockplus-android-settings/build.gradle:52: configFile =
file("${rootProject.rootDir}/third_party/checkstyle/java/rules/eyeo_checks.xml")
Are we using the other checks files? It seems that we're only using the
"eyeo_checks"

https://codereview.adblockplus.org/29347926/diff/29412565/third_party/checkst...
File third_party/checkstyle/java/rules/eyeo_checks.xml (right):

https://codereview.adblockplus.org/29347926/diff/29412565/third_party/checkst...
third_party/checkstyle/java/rules/eyeo_checks.xml:98: <!-- See
http://checkstyle.sf.net/config_import.html -->
As I stated in patch set 1, This link is broken, it should be replaced by
http://checkstyle.sourceforge.net/config_imports.html

https://codereview.adblockplus.org/29347926/diff/29412565/third_party/checkst...
third_party/checkstyle/java/rules/eyeo_checks.xml:179: <!-- <module
name="FinalParameters"/> -->
Shouldn't we cleanup the file and remove the modules which we aren't using?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5