fixes for AbstractClassCouplingCheck#69
Conversation
lack resources and time force me to be more strict with PR. Ok, thanks for you time to figure out and sum-up changes, I will put this in queue with hight priority. |
|
Why travis build status is not displaying here? I've made a PR to spring project and travis status is in place. |
|
does it make sense to print content of DEFAULT_EXCLUDED_CLASSES in HTML doc page to let user know what is excluded by default and where is no way change this ? the better way is to set content of DEFAULT_EXCLUDED_CLASSES as default value for mExcludedClasses, and user will have full control over this Check, and we provide only reasonable configuration for default. |
No idea, it worked well before ... https://travis-ci.org/checkstyle/checkstyle/builds, strange. |
|
It is there - https://travis-ci.org/checkstyle/checkstyle/pull_requests and https://travis-ci.org/checkstyle/checkstyle/builds/14673416 just is not displayed on this page. |
|
The original intent to make a separate list of user-defined excluded classes was to not force users to retype a really long list of classes excluded by default in their configurations. Also the default list is intended to be really "core" and "undisputable". However it is not really such - so long list of Execeptions (were there before these issues), Locale class that was in the issue and that I have not added to this PR and such classes as SortedMap, Queue and Deque are debatable as the "core of the language". About adding the last three collections - adding LinkedList to the ignored set of classes and not adding Queue and Deque may lead to usage of LinkedList type in too many places to not introduce these two "additional" types into the code - however without mentioning them they are really introduced since LinkedList implements both these interfaces. As a side note - I hardly can imagine myself using this check. |
nobody will retype this, defined ones and for ages, any very picky developer who do care about quality on this level will extend it. even after Java version X release, there will be no reason to update that Check again. User just extend it on his side.
I do not believe that there is a project in Java where all Checks are enabled and works on default values ... all of them adjusted to team experience and meaning of "Clear code". So lets avoid/reduce amount of undisputable and let user decide what is right.
I am with you, but may be I am too young/immature to understand its value. But Checkstyle provide algorithm and user decide how to use/misuse it. Summary: I am ok to apply your patch, this Check is not used at all, and we will not do harm to it. |
|
Amended the changes to the commit. Travis builds - https://travis-ci.org/checkstyle/checkstyle/builds/14805670 |
fixes for AbstractClassCouplingCheck
|
@isopov, thanks a lot for fix and link to travis. Merged. |
Fix for issues #31 and #33
Changes from the issue:
"Locale" is not added to the default set of classes to ignore, since it seems not very "core" to me. In addition to Map, List, HashMap and ArrayList collections Deque, Queue, LinkedList, Set, HashSet, SortedSet, TreeSet, SortedMap, TreeMap are added. Also StringBuilder was added since StringBuffer was already there.
For reference this is closely connected with #36 (another issue, that should be rejected IMO), #34, #35, #37 (PRs for these issues that were rejected for bureaucratic reasons).
I feel really strange rejecting contributions from the author of the book about Maven that I used to learn. So lets push this changes if he needs them.