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

Issue 29347926: Issue 4248 - Add codestyle check

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 1 month ago by anton
Modified:
3 months, 2 weeks 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: 10

Patch Set 5 : fixed broken url in rules #

Patch Set 6 : added test rules set, apply test rules for testing #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+1127 lines, -82 lines) Patch
M build.gradle View 1 2 3 1 chunk +7 lines, -0 lines 0 comments Download
M libadblockplus-android-settings/build.gradle View 1 2 3 4 5 2 chunks +22 lines, -2 lines 0 comments Download
M libadblockplus-android-tests/build.gradle View 1 2 3 4 5 1 chunk +52 lines, -30 lines 1 comment Download
M libadblockplus-android-webview/build.gradle View 1 2 3 4 5 1 chunk +42 lines, -20 lines 0 comments Download
M libadblockplus-android-webviewapp/build.gradle View 1 2 3 4 5 2 chunks +22 lines, -1 line 0 comments Download
M libadblockplus-android/build.gradle View 1 2 3 4 5 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 0 comments Download
A third_party/checkstyle/java/rules/eyeo_production_checks.xml View 1 2 3 4 5 1 chunk +185 lines, -0 lines 1 comment Download
A third_party/checkstyle/java/rules/eyeo_test_checks.xml View 1 2 3 4 5 1 chunk +188 lines, -0 lines 3 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: 19
anton
header.txt is file header that every java file should have. (for some reason Rietveld shows ...
1 year, 1 month 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) ...
1 year, 1 month 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 ...
1 year, 1 month 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 ...
9 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 ...
9 months, 2 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 ...
9 months 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: > ...
9 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: > ...
9 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: ...
8 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 ...
4 months, 2 weeks 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 ...
4 months, 1 week 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 > ...
4 months, 1 week ago (2017-04-14 12:02:52 UTC) #12
diegocarloslima
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 ...
3 months, 3 weeks ago (2017-04-27 12:58:45 UTC) #13
anton
uploaded new patch set 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/27 12:58:44, diegocarloslima ...
3 months, 3 weeks ago (2017-04-28 06:05:06 UTC) #14
diegocarloslima
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/28 06:05:06, anton wrote: > On 2017/04/27 ...
3 months, 3 weeks ago (2017-04-28 11:27:38 UTC) #15
anton
uploaded new patch set https://codereview.adblockplus.org/29347926/diff/29424692/third_party/checkstyle/java/rules/eyeo_test_checks.xml File third_party/checkstyle/java/rules/eyeo_test_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29424692/third_party/checkstyle/java/rules/eyeo_test_checks.xml#newcode93 third_party/checkstyle/java/rules/eyeo_test_checks.xml:93: <property name="format" value="^[a-z][_a-zA-Z0-9]*$"/> changed format ...
3 months, 3 weeks ago (2017-04-28 11:55:53 UTC) #16
diegocarloslima
On 2017/04/28 11:55:53, anton wrote: > uploaded new patch set > > https://codereview.adblockplus.org/29347926/diff/29424692/third_party/checkstyle/java/rules/eyeo_test_checks.xml > File ...
3 months, 3 weeks ago (2017-04-28 12:28:10 UTC) #17
Felix Dahlke
Looks promising, a few comments. But most importantly: I didn't review the actual check files ...
3 months, 2 weeks ago (2017-05-09 08:29:36 UTC) #18
anton
3 months, 2 weeks ago (2017-05-10 12:06:58 UTC) #19
On 2017/05/09 08:29:36, Felix Dahlke wrote:
> Looks promising, a few comments.
> 
> But most importantly: I didn't review the actual check files yet. It'd be a
lot
> easier for me to first know what kind of warnings/errors this would produce in
> our various repositories.
> 
> How we did it for the ESLint stuff in our JavaScript code bases is, Dave
> uploaded two reviews:
> 
> 1. One with the actual configs, added to codingtools.
> 2. One that adds code checking to our JS repos, with all warnings/errors
> addressed.
> 

The same story - all files in "third_party" directory are meant to be stored in
separate (new or existing) Mercurial repository.
It' task (1).

Modified build.gradle files are task (2), which actually adds checking.

> If (2) seems like too much effort now, maybe we should start with something
> small, like ABP for SBrowser?

Sure, we can start with ABP for SBrowser, but i don't see any problem with
starting for libadblockplus-android.

> 
>
https://codereview.adblockplus.org/29347926/diff/29424692/libadblockplus-andr...
> File libadblockplus-android-tests/build.gradle (right):
> 
>
https://codereview.adblockplus.org/29347926/diff/29424692/libadblockplus-andr...
> libadblockplus-android-tests/build.gradle:5: repositories {
> Seems we're moving from two space indents to four space indents in Gradle
code.
> Why is that?

Agree we should have consistent code style for build.gradle files too, but what
is it?
`build.gradle` files are actually Groovy-code that is based on Java, so should
we format it like it's Java.
It will look very strange with brackets on new line each though.

> 
>
https://codereview.adblockplus.org/29347926/diff/29424692/third_party/checkst...
> File third_party/checkstyle/java/rules/eyeo_production_checks.xml (right):
> 
>
https://codereview.adblockplus.org/29347926/diff/29424692/third_party/checkst...
> third_party/checkstyle/java/rules/eyeo_production_checks.xml:1: <?xml
> version="1.0"?>
> We will this file in multiple repositories, can we instead add it to the
> codingtools repository? That's also where we have our ESLint configuration.

Yes it was meant to be exactly in the way you ask (read above).

> 
> The same goes for the other rule files.
Sign in to reply to this message.

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