Difference between revisions of "SMILA/Discussions/Checkstyle"

From Eclipsepedia

Jump to: navigation, search
(SMILA Checkstyle configuration changes)
 
(2 intermediate revisions by 2 users not shown)
Line 1: Line 1:
 
== SMILA Checkstyle configuration changes ==
 
== SMILA Checkstyle configuration changes ==
 +
 +
=== compromise ===
 +
 +
1. brox_checks.xml is replaced by two new checkstyle files: smila_checkstyle.xml, smila-test_checkstyle.xml (for test bundles)
 +
 +
2. Apply changes where everyone agreed: d, e, t, t1-t5, t7
 +
 +
3. Regarding a/b: Compromise to reduce javadoc overhead is, to not realize a/b but to apply t2-t4 to _all_ (private and public) methods, not only in tests. (BTW, this can not only be applied for private methods cause it seems that you can't have different checkstyle settings for private and public methods.)
 +
 +
4. Regarding c: Majority has decided ;)  Maybe we can use these other complexity measures Jürgen mentioned instead (CyclomaticComplexity / NPathComplexity).
 +
 +
5. Regarding f: Majority has decided again. Daniel should be convinced with some Donuts... ;)
 +
 +
 +
=== discussion ===
  
 
I'd like to suggest some changes for the checkstyle settings of our current checkstyle configuration file (''brox_checks.xml'') here. IMHO, the current configuration is too strict, produces much javadoc overhead and reduces developer acceptance.
 
I'd like to suggest some changes for the checkstyle settings of our current checkstyle configuration file (''brox_checks.xml'') here. IMHO, the current configuration is too strict, produces much javadoc overhead and reduces developer acceptance.
Line 55: Line 70:
 
! Jürgen Schumacher
 
! Jürgen Schumacher
 
! Igor Novakovic
 
! Igor Novakovic
 +
! Daniel Stucky
 
|-
 
|-
 
| a)
 
| a)
Line 62: Line 78:
 
| -1
 
| -1
 
| -1
 
| -1
 +
| 0
 
|-
 
|-
 
| b)
 
| b)
Line 69: Line 86:
 
| -1  
 
| -1  
 
| -1
 
| -1
 +
| 0
 
|-
 
|-
 
| c)
 
| c)
Line 76: Line 94:
 
| -1
 
| -1
 
| 0
 
| 0
 +
| +1
 
|-
 
|-
 
| d)
 
| d)
 
| +1
 
| +1
 
| +1  
 
| +1  
 +
| +1
 
| +1
 
| +1
 
| +1
 
| +1
Line 90: Line 110:
 
| +1
 
| +1
 
| +1  
 
| +1  
 +
| +1
 
|-
 
|-
 
| f)
 
| f)
Line 97: Line 118:
 
| +1
 
| +1
 
| +1
 
| +1
 +
| --1
 
|-
 
|-
 
| t)
 
| t)
Line 104: Line 126:
 
| +1
 
| +1
 
| +1  
 
| +1  
 +
| +1
 
|-
 
|-
 
| t1)
 
| t1)
Line 110: Line 133:
 
| +1
 
| +1
 
| +1
 
| +1
 +
| 0
 
| 0
 
| 0
 
|-
 
|-
Line 117: Line 141:
 
| 0
 
| 0
 
| +1
 
| +1
 +
| 0
 
| 0
 
| 0
 
|-
 
|-
Line 124: Line 149:
 
| +1
 
| +1
 
| +1
 
| +1
 +
| 0
 
| 0
 
| 0
 
|-
 
|-
Line 131: Line 157:
 
| 0  
 
| 0  
 
| +1
 
| +1
 +
| 0
 
| 0
 
| 0
 
|-
 
|-
Line 138: Line 165:
 
| 0
 
| 0
 
| +1
 
| +1
 +
| 0
 
| 0
 
| 0
 
|-
 
|-
Line 146: Line 174:
 
| -1
 
| -1
 
| -1
 
| -1
 +
| 0
 
|-
 
|-
 
| t7)
 
| t7)
 
| +1
 
| +1
 
| +1
 
| +1
 +
| 0
 
| 0
 
| 0
 
| 0
 
| 0
 
| 0
 
| 0
 
|}
 
|}
 +
 +
[[Category:SMILA]]

Latest revision as of 07:06, 25 January 2012

[edit] SMILA Checkstyle configuration changes

[edit] compromise

1. brox_checks.xml is replaced by two new checkstyle files: smila_checkstyle.xml, smila-test_checkstyle.xml (for test bundles)

2. Apply changes where everyone agreed: d, e, t, t1-t5, t7

3. Regarding a/b: Compromise to reduce javadoc overhead is, to not realize a/b but to apply t2-t4 to _all_ (private and public) methods, not only in tests. (BTW, this can not only be applied for private methods cause it seems that you can't have different checkstyle settings for private and public methods.)

4. Regarding c: Majority has decided ;) Maybe we can use these other complexity measures Jürgen mentioned instead (CyclomaticComplexity / NPathComplexity).

5. Regarding f: Majority has decided again. Daniel should be convinced with some Donuts... ;)


[edit] discussion

I'd like to suggest some changes for the checkstyle settings of our current checkstyle configuration file (brox_checks.xml) here. IMHO, the current configuration is too strict, produces much javadoc overhead and reduces developer acceptance.

I tried to restrict my suggestions to those that should be possible, according to the Checkstyle 5.3 documentation.

I'll label the suggestions with a,b,c,etc. so we can make a voting in the table below. Feel free to add some more suggestions.

Furthermore, I'd suggest a different checkstyle configuration for test bundles with some additional changes, see t1,t2,... below.


Suggestions:

a) exclude "private" methods from javadoc check.

(JS) IMHO it would be sufficient to not require to document every parameter, exception or return value explicitly. We could change this for all kind of methods: A good descriptive text plus good variable names should usually be sufficient as method documentation.

b) exclude "private" variables from javadoc check.

c) remove check for allowed method length (IMHO the methold length is just a hint for too much complexity in a method, but not a real proof - so this should be decided by the developer)

(JS) there are some other "complexity" metrics in checkstyle, maybe we can use these (additionally). On the other hand, the method length *IS* a useful metric: If a method gets too long, it will be hard to understand, because you are not able to get an overview about it. Next, such method tend to need (and have) a lot of inline comments, which make them even longer (Instead of commenting a part of a long method it's better to extract a method - the method name is a comment by itself!). So I think this warning *IS* valid and the developer should think about it. However, for test bundles this limit can be removed.

d) javadoc methods: allow documented exceptions that are not declared if they are a subclass of java.lang.RuntimeException.

e) javadoc methods: allow documented exceptions that are subclass of one of declared exception.

f) allow inline conditionals

t) different checkstyle configuration for test bundles

t1) remove check for "magic numbers".

t2) javadoc methods: allow missing param tags.

t3) javadoc methods: allow missing throws tag.

t4) javadoc methods: allow missing return tag.

t5) remove javadoc check for variables

t6) have no checksstyle for test classes at all.
TM: maybe a bit harsh but i think it still would be OK.
AW: I thought about that too, but finally I'd say that tests are too important to completely get rid of checks.

t7) allow '_' in method and class names (i use this to separate and group the test cases and make them more visible/readable, e.g. Solr_SearchPipelet_ScenarioA_Test, test_feature_variantA_OK, test_feature_variantA_Fail, ... )

Votings:

Andreas Weber Thomas Menzel Andreas Schank Jürgen Schumacher Igor Novakovic Daniel Stucky
a) +1 +1 -1 -1 -1 0
b) +1 +1 -1 -1 -1 0
c) +1 +1 0 -1 0 +1
d) +1 +1 +1 +1 +1 +1
e) +1 +1 +1 +1 +1 +1
f) +1 ++1 ;) +1 +1 +1 --1
t) +1 +1 +1 +1 +1 +1
t1) +1 +1 +1 +1 0 0
t2) +1 +1 0 +1 0 0
t3) +1 +1 +1 +1 0 0
t4) +1 +1 0 +1 0 0
t5) +1 +1 0 +1 0 0
t6) -1 0 -1 -1 -1 0
t7) +1 +1 0 0 0 0