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

Issue 29347926: Issue 4248 - Add codestyle check

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 4 months ago by anton
Modified:
2 months ago
Visibility:
Public.

Description

Issue 4248 - Add codestyle check #depends on https://codereview.adblockplus.org/29567648/ ('dependencies' file to be updated)

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

Patch Set 7 : moved 'third_party' to review https://codereview.adblockplus.org/29567648/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+188 lines, -74 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 0 comments 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

Messages

Total messages: 20
anton
header.txt is file header that every java file should have. (for some reason Rietveld shows ...
1 year, 4 months 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, 4 months 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, 4 months 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 ...
1 year, 1 month 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 ...
1 year, 1 month 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 ...
1 year 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: > ...
1 year 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: > ...
1 year 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: ...
1 year 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 ...
8 months, 1 week 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 ...
8 months 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 > ...
8 months 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 ...
7 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 ...
7 months, 2 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 ...
7 months, 2 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 ...
7 months, 2 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 ...
7 months, 2 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 ...
7 months, 1 week ago (2017-05-09 08:29:36 UTC) #18
anton
On 2017/05/09 08:29:36, Felix Dahlke wrote: > Looks promising, a few comments. > > But ...
7 months, 1 week ago (2017-05-10 12:06:58 UTC) #19
anton
2 months, 1 week ago (2017-10-06 11:35:48 UTC) #20
On 2017/05/10 12:06:58, anton wrote:
> 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.

Updated patch set and moved third_party to separate review (separate repo)
Sign in to reply to this message.

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