Issue 29347926: Issue 4248 - Add codestyle check
DescriptionIssue 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/ #
MessagesTotal messages: 20
header.txt is file header that every java file should have. (for some reason Rietveld shows it was edited but it's was created from scratch). third_party/checkstyle is meant to be stored in separate repo and used across the projects and even languages. to be moved to repo and removed from here after approval. google_checks and sun_checks are extracted from .jar. mozilla_checks is saved from https://github.com/mozilla-services/mozilla-style/blob/master/java/checkstyle... with minor modifications (deprecated rules are removed). eyeo_rules is created to reflect our own codestyle and is a subject to change (to be checked carefully by Rene or some senior person). use "ant checkstyle" to perform code style checking.
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:67: <module name="Header"> added this rule (pass filename as argument) since we are required to add it in each java file https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:76: <!-- <module name="JavadocMethod"/> --> i know i need to remove commented out lines, but left here just to illustrate it's available. https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:103: <property name="groups" value="org.mozilla,com,net,org,android,java,javax"/> according to mozilla's style org.mozilla should be the first? do we need to have the same rule? https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:138: <property name="option" value="nl"/> according to eyeo's style on new line, f.e. void func() { ... BTW, what is the reason to skip traditional java-style like void func() { ... https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:147: <module name="AvoidInlineConditionals"/> is it bad to use (case?positive_branch:negative_branch)? http://checkstyle.sourceforge.net/config_coding.html#AvoidInlineConditionals actually i do use it time to time https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:174: <module name="UpperEll"/> does it make sense to write L instead of l? f.e. 10L vs. 10l
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:110: <property name="max" value="100"/> according to mozilla's style one could have lines a bit longer than 100 chars if wrapping hurts readability. Using 100 for now https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:125: <module name="WhitespaceAfter"/> could be changed from default to COMMA and SEMI only: http://checkstyle.sourceforge.net/config_whitespace.html#WhitespaceAfter Otherwise we will have error after (int) typecast in Filter.java f.e.: @Override public int hashCode() { return (int)(this.ptr >> 32) * (int)this.ptr; } https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:130: <module name="ModifierOrder"/> currently we have "final static" almost in most places (f.e.), but "static final" is recommended by ModifierOrder: http://checkstyle.sourceforge.net/config_modifier.html totally up to us to decide what code style we support. https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:153: <module name="MagicNumber"/> a bit discussed it with Rene. In tests we can have this (f.e. Thread.sleep(1000)), but it seems to be okay. I believe it's better to have some comment about why 1000 is appropriate here or why we should wait for such time. Anyway this is not allowed in production code but seems to be reasonable for tests. After brief discussion we decided to have the same rules set for both production code and test code and have strict rules for tests too. To be discussed more if/when needed. https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:173: <module name="TodoComment"/> afaik it's not allowed to add TODO: but actually we have some places with TODOs. I believe we should have this rule and fix this in the code
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:46: <module name="NewlineAtEndOfFile"/> I think that it would be good to organize our file by grouping all modules declarations that belongs to the same section in checkstyle.sf.net documentation. For instance, this module should be together with all other modules that belongs to the misc category (http://checkstyle.sourceforge.net/config_misc.html) https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:94: <!-- See http://checkstyle.sf.net/config_import.html --> This link is broken, it should be replaced by http://checkstyle.sourceforge.net/config_imports.html https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:103: <property name="groups" value="org.mozilla,com,net,org,android,java,javax"/> On 2016/07/19 06:45:44, anton wrote: > according to mozilla's style org.mozilla should be the first? do we need to have > the same rule? We usually keep like that for files that are maintained by Mozilla, but I don't think that it applies to our files https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:125: <module name="WhitespaceAfter"/> On 2016/07/21 10:12:47, anton wrote: > could be changed from default to COMMA and SEMI only: > http://checkstyle.sourceforge.net/config_whitespace.html#WhitespaceAfter > > Otherwise we will have error after (int) typecast in Filter.java f.e.: > @Override > public int hashCode() > { > return (int)(this.ptr >> 32) * (int)this.ptr; > } I personally prefer a white space after a typecast, but no strong opinion about that https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:130: <module name="ModifierOrder"/> On 2016/07/21 10:12:46, anton wrote: > currently we have "final static" almost in most places (f.e.), but "static > final" is recommended by ModifierOrder: > http://checkstyle.sourceforge.net/config_modifier.html > > totally up to us to decide what code style we support. I would go for the recommended one, 'static final' https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:147: <module name="AvoidInlineConditionals"/> On 2016/07/19 06:45:44, anton wrote: > is it bad to use (case?positive_branch:negative_branch)? > http://checkstyle.sourceforge.net/config_coding.html#AvoidInlineConditionals > > actually i do use it time to time I also use it from time to time. I think it depends on how big the inline conditional is. If is too big with many operands, it might hurt the readability https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:174: <module name="UpperEll"/> On 2016/07/19 06:45:44, anton wrote: > does it make sense to write L instead of l? f.e. 10L vs. 10l I think it does, helps to maintain good readability and avoid misleadings
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:46: <module name="NewlineAtEndOfFile"/> On 2016/11/03 14:00:51, diegocarloslima wrote: > I think that it would be good to organize our file by grouping all modules > declarations that belongs to the same section in http://checkstyle.sf.net > documentation. For instance, this module should be together with all other > modules that belongs to the misc category > (http://checkstyle.sourceforge.net/config_misc.html) No problem https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:94: <!-- See http://checkstyle.sf.net/config_import.html --> On 2016/11/03 14:00:51, diegocarloslima wrote: > This link is broken, it should be replaced by > http://checkstyle.sourceforge.net/config_imports.html Acknowledged. https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:130: <module name="ModifierOrder"/> On 2016/11/03 14:00:51, diegocarloslima wrote: > On 2016/07/21 10:12:46, anton wrote: > > currently we have "final static" almost in most places (f.e.), but "static > > final" is recommended by ModifierOrder: > > http://checkstyle.sourceforge.net/config_modifier.html > > > > totally up to us to decide what code style we support. > > I would go for the recommended one, 'static final' Agree, i prefer "static final" too. Rene? https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:147: <module name="AvoidInlineConditionals"/> On 2016/11/03 14:00:50, diegocarloslima wrote: > On 2016/07/19 06:45:44, anton wrote: > > is it bad to use (case?positive_branch:negative_branch)? > > http://checkstyle.sourceforge.net/config_coding.html#AvoidInlineConditionals > > > > actually i do use it time to time > > I also use it from time to time. I think it depends on how big the inline > conditional is. If is too big with many operands, it might hurt the readability Agree. I think we need one person (like Rene or Felix) to take the solution as it's a matter of tastes IMHO. https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:174: <module name="UpperEll"/> On 2016/11/03 14:00:51, diegocarloslima wrote: > On 2016/07/19 06:45:44, anton wrote: > > does it make sense to write L instead of l? f.e. 10L vs. 10l > > I think it does, helps to maintain good readability and avoid misleadings Usually it's highlighted by IDE so it's not a problem until you write in notepad. However i would agree L is slightly better for readability
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:125: <module name="WhitespaceAfter"/> On 2016/11/03 14:00:50, diegocarloslima wrote: > On 2016/07/21 10:12:47, anton wrote: > > could be changed from default to COMMA and SEMI only: > > http://checkstyle.sourceforge.net/config_whitespace.html#WhitespaceAfter > > > > Otherwise we will have error after (int) typecast in Filter.java f.e.: > > @Override > > public int hashCode() > > { > > return (int)(this.ptr >> 32) * (int)this.ptr; > > } > > I personally prefer a white space after a typecast, but no strong opinion about > that I think we have spaces after type casts ... but I'm not 100% sure. https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:130: <module name="ModifierOrder"/> On 2016/11/07 07:40:44, anton wrote: > On 2016/11/03 14:00:51, diegocarloslima wrote: > > On 2016/07/21 10:12:46, anton wrote: > > > currently we have "final static" almost in most places (f.e.), but "static > > > final" is recommended by ModifierOrder: > > > http://checkstyle.sourceforge.net/config_modifier.html > > > > > > totally up to us to decide what code style we support. > > > > I would go for the recommended one, 'static final' > > Agree, i prefer "static final" too. Rene? Yes, I also strongly prefer using the recommended way here, so it should be 'static final'. https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:138: <property name="option" value="nl"/> On 2016/07/19 06:45:45, anton wrote: > according to eyeo's style on new line, f.e. > > void func() > { > ... > > BTW, what is the reason to skip traditional java-style like > > void func() { > ... Well, I could list reasons why having the curlies on the next line is better than on the same line, but it doesn't matter in the end. It is like it is, and I love it. ^^ https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:147: <module name="AvoidInlineConditionals"/> On 2016/11/07 07:40:44, anton wrote: > On 2016/11/03 14:00:50, diegocarloslima wrote: > > On 2016/07/19 06:45:44, anton wrote: > > > is it bad to use (case?positive_branch:negative_branch)? > > > http://checkstyle.sourceforge.net/config_coding.html#AvoidInlineConditionals > > > > > > actually i do use it time to time > > > > I also use it from time to time. I think it depends on how big the inline > > conditional is. If is too big with many operands, it might hurt the > readability > > Agree. I think we need one person (like Rene or Felix) to take the solution as > it's a matter of tastes IMHO. I don't think we should disable the ternary operator. There are various situations where this operator makes things a lot more readable instead of having 9 lines of code (if/else). Also, this goes nicely with 'final' variables, makes more sense than using if/else. https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:173: <module name="TodoComment"/> On 2016/07/21 10:12:47, anton wrote: > afaik it's not allowed to add TODO: but actually we have some places with TODOs. > I believe we should have this rule and fix this in the code Yes, we should not use TODO/FIXME/XXX comments in code and use the issue tracker instead. https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:174: <module name="UpperEll"/> On 2016/11/07 07:40:44, anton wrote: > On 2016/11/03 14:00:51, diegocarloslima wrote: > > On 2016/07/19 06:45:44, anton wrote: > > > does it make sense to write L instead of l? f.e. 10L vs. 10l > > > > I think it does, helps to maintain good readability and avoid misleadings > > Usually it's highlighted by IDE so it's not a problem until you write in > notepad. However i would agree L is slightly better for readability Yes, 'L' is a lot better readable than 'l'.
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:130: <module name="ModifierOrder"/> On 2016/11/21 11:44:19, René Jeschke wrote: > On 2016/11/07 07:40:44, anton wrote: > > On 2016/11/03 14:00:51, diegocarloslima wrote: > > > On 2016/07/21 10:12:46, anton wrote: > > > > currently we have "final static" almost in most places (f.e.), but "static > > > > final" is recommended by ModifierOrder: > > > > http://checkstyle.sourceforge.net/config_modifier.html > > > > > > > > totally up to us to decide what code style we support. > > > > > > I would go for the recommended one, 'static final' > > > > Agree, i prefer "static final" too. Rene? > > Yes, I also strongly prefer using the recommended way here, so it should be > 'static final'. Acknowledged. https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:138: <property name="option" value="nl"/> On 2016/11/21 11:44:20, René Jeschke wrote: > On 2016/07/19 06:45:45, anton wrote: > > according to eyeo's style on new line, f.e. > > > > void func() > > { > > ... > > > > BTW, what is the reason to skip traditional java-style like > > > > void func() { > > ... > > Well, I could list reasons why having the curlies on the next line is better > than on the same line, but it doesn't matter in the end. It is like it is, and I > love it. ^^ No problem. https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:147: <module name="AvoidInlineConditionals"/> On 2016/11/21 11:44:19, René Jeschke wrote: > On 2016/11/07 07:40:44, anton wrote: > > On 2016/11/03 14:00:50, diegocarloslima wrote: > > > On 2016/07/19 06:45:44, anton wrote: > > > > is it bad to use (case?positive_branch:negative_branch)? > > > > > http://checkstyle.sourceforge.net/config_coding.html#AvoidInlineConditionals > > > > > > > > actually i do use it time to time > > > > > > I also use it from time to time. I think it depends on how big the inline > > > conditional is. If is too big with many operands, it might hurt the > > readability > > > > Agree. I think we need one person (like Rene or Felix) to take the solution as > > it's a matter of tastes IMHO. > > I don't think we should disable the ternary operator. There are various > situations where this operator makes things a lot more readable instead of > having 9 lines of code (if/else). > > Also, this goes nicely with 'final' variables, makes more sense than using > if/else. Acknowledged. https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:173: <module name="TodoComment"/> On 2016/11/21 11:44:20, René Jeschke wrote: > On 2016/07/21 10:12:47, anton wrote: > > afaik it's not allowed to add TODO: but actually we have some places with > TODOs. > > I believe we should have this rule and fix this in the code > > Yes, we should not use TODO/FIXME/XXX comments in code and use the issue tracker > instead. Acknowledged. https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... third_party/checkstyle/java/rules/eyeo_checks.xml:174: <module name="UpperEll"/> On 2016/11/21 11:44:20, René Jeschke wrote: > On 2016/11/07 07:40:44, anton wrote: > > On 2016/11/03 14:00:51, diegocarloslima wrote: > > > On 2016/07/19 06:45:44, anton wrote: > > > > does it make sense to write L instead of l? f.e. 10L vs. 10l > > > > > > I think it does, helps to maintain good readability and avoid misleadings > > > > Usually it's highlighted by IDE so it's not a problem until you write in > > notepad. However i would agree L is slightly better for readability > > Yes, 'L' is a lot better readable than 'l'. Acknowledged.
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... 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: > On 2016/07/19 06:45:44, anton wrote: > > according to mozilla's style org.mozilla should be the first? do we need to > have > > the same rule? > > We usually keep like that for files that are maintained by Mozilla, but I don't > think that it applies to our files As this is for having 'in-house' imports at the top, we can remove the 'org.mozilla' part. AFAIK we do not have rules for our imports, but if we want to go the Mozilla way here, then we could replace 'org.mozilla' with 'org.adblockplus'.
https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... File third_party/checkstyle/java/rules/eyeo_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29347927/third_party/checkst... 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: > On 2016/11/03 14:00:51, diegocarloslima wrote: > > On 2016/07/19 06:45:44, anton wrote: > > > according to mozilla's style org.mozilla should be the first? do we need to > > have > > > the same rule? > > > > We usually keep like that for files that are maintained by Mozilla, but I > don't > > think that it applies to our files > > As this is for having 'in-house' imports at the top, we can remove the > 'org.mozilla' part. AFAIK we do not have rules for our imports, but if we want > to go the Mozilla way here, then we could replace 'org.mozilla' with > 'org.adblockplus'. Acknowledged.
https://codereview.adblockplus.org/29347926/diff/29366728/third_party/checkst... File third_party/checkstyle/java/rules/google_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29366728/third_party/checkst... third_party/checkstyle/java/rules/google_checks.xml:6: <!-- Maybe would be good the put the link of where this checkstyle came from (I assume it was from https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/googl...) , as you did in the mozilla one https://codereview.adblockplus.org/29347926/diff/29366728/third_party/checkst... File third_party/checkstyle/java/rules/sun_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29366728/third_party/checkst... third_party/checkstyle/java/rules/sun_checks.xml:7: Maybe would be good the put the link of where this checkstyle came from (I assume it was from https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/sun_c...) , as you did in the mozilla one
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 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? https://codereview.adblockplus.org/29347926/diff/29412565/third_party/checkst... File third_party/checkstyle/java/res/header.txt (right): https://codereview.adblockplus.org/29347926/diff/29412565/third_party/checkst... third_party/checkstyle/java/res/header.txt:16: */ rietveld bug: the file is new and there is no previous revision
On 2017/04/07 13:46:38, diegocarloslima wrote: > https://codereview.adblockplus.org/29347926/diff/29366728/third_party/checkst... > File third_party/checkstyle/java/rules/google_checks.xml (right): > > https://codereview.adblockplus.org/29347926/diff/29366728/third_party/checkst... > third_party/checkstyle/java/rules/google_checks.xml:6: <!-- > Maybe would be good the put the link of where this checkstyle came from (I > assume it was from > https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/googl...) > , as you did in the mozilla one > > https://codereview.adblockplus.org/29347926/diff/29366728/third_party/checkst... > File third_party/checkstyle/java/rules/sun_checks.xml (right): > > https://codereview.adblockplus.org/29347926/diff/29366728/third_party/checkst... > third_party/checkstyle/java/rules/sun_checks.xml:7: > Maybe would be good the put the link of where this checkstyle came from (I > assume it was from > https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/sun_c...) > , as you did in the mozilla one 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/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?
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 wrote: > 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 tool version is just a bit. The main purpose is task arguments - paths, files, etc. what about rules for tests - how do you think should we have separate rules files for it? as it's obviously that some rules are not respected. eg. "Magic numbers" (delayes, expected values, etc) 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") On 2017/04/27 12:58:44, diegocarloslima wrote: > Are we using the other checks files? It seems that we're only using the > "eyeo_checks" yes, here we use 'eyeo_checks' only. but since it's going to be external repository i guess another files (like google, sun rules files) will be handy too 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 --> On 2017/04/27 12:58:45, diegocarloslima wrote: > As I stated in patch set 1, This link is broken, it should be replaced by > http://checkstyle.sourceforge.net/config_imports.html Acknowledged.
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 12:58:44, diegocarloslima wrote: > > 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 > > tool version is just a bit. The main purpose is task arguments - paths, files, > etc. > > what about rules for tests - how do you think should we have separate rules > files for it? as it's obviously that some rules are not respected. eg. "Magic > numbers" (delayes, expected values, etc) Yes, It makes sense to have some special rules for testing
uploaded new patch set https://codereview.adblockplus.org/29347926/diff/29424692/third_party/checkst... File third_party/checkstyle/java/rules/eyeo_test_checks.xml (right): https://codereview.adblockplus.org/29347926/diff/29424692/third_party/checkst... third_party/checkstyle/java/rules/eyeo_test_checks.xml:93: <property name="format" value="^[a-z][_a-zA-Z0-9]*$"/> changed format for tests - added underline character allowed: see https://developer.android.com/training/testing/unit-testing/instrumented-unit...: `public void logHistory_ParcelableWriteRead()` https://codereview.adblockplus.org/29347926/diff/29424692/third_party/checkst... third_party/checkstyle/java/rules/eyeo_test_checks.xml:143: <!--<module name="EmptyBlock"/>--> for test impl we can have empty blocks https://codereview.adblockplus.org/29347926/diff/29424692/third_party/checkst... third_party/checkstyle/java/rules/eyeo_test_checks.xml:160: <!--<module name="MagicNumber"/>--> not applied for the tests
On 2017/04/28 11:55:53, anton wrote: > uploaded new patch set > > https://codereview.adblockplus.org/29347926/diff/29424692/third_party/checkst... > File third_party/checkstyle/java/rules/eyeo_test_checks.xml (right): > > https://codereview.adblockplus.org/29347926/diff/29424692/third_party/checkst... > third_party/checkstyle/java/rules/eyeo_test_checks.xml:93: <property > name="format" value="^[a-z][_a-zA-Z0-9]*$"/> > changed format for tests - added underline character allowed: > see > https://developer.android.com/training/testing/unit-testing/instrumented-unit...: > `public void logHistory_ParcelableWriteRead()` > > https://codereview.adblockplus.org/29347926/diff/29424692/third_party/checkst... > third_party/checkstyle/java/rules/eyeo_test_checks.xml:143: <!--<module > name="EmptyBlock"/>--> > for test impl we can have empty blocks > > https://codereview.adblockplus.org/29347926/diff/29424692/third_party/checkst... > third_party/checkstyle/java/rules/eyeo_test_checks.xml:160: <!--<module > name="MagicNumber"/>--> > not applied for the tests LGTM
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. If (2) seems like too much effort now, maybe we should start with something small, like ABP for SBrowser? 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? 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. The same goes for the other rule files.
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.
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) 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
