I've now applied the patch locally and done a detailed review. I'm posting this a bit outside the context of recent discussions to simply state my present opinion after looking into the patch.
Generally, this is a big improvement. So thanks, Glenn, for your work here! I'm also not particularly happy about the //CS* comments. To a certain degree I think I could live with them. A count shows 279 usages. I think that may be a tad too much. Maybe we can find something in between, like making more use of the "error" severity. Most checks are just warnings right now. So using errors will make it easier to enforce at least the rules most important to us. I've also experimented with the regular expressions: <module name="ConstantNameCheck"> <property name="format" value="(^[A-Z](_?[A-Z0-9]+)*$)|log"/> <property name="severity" value="warning"/> </module> This should already make several such //CS comments unnecessary. There are other comments referencing ConstantNameCheck where we should rather convert the name to upper case. That will cut down on these even further. Like Chris suggested, we could then even decide to live with a few warnings as long as we increase the severity of the most important rules and set up a no-go policy for "errors". I saw some changes in LineBreakPairTable.txt and LineBreakUtils.java. Glenn, was that an accidental overflow from your work on the new features? I have no problem with the sometimes rather generic Javadoc comments. Every committer is invited to improve on those as he's passing over particular code parts. I know that we have quite a bit of outdated documentation already in our Javadocs. So these comments don't make the situation worse IMO. The only thing we can do is gradually improve. But at least the generic javadocs lets us cut down on the number of warnings so we can really focus to improve there rather than capitulate before thousands of warnings. Finally a nit: some files have got method signatures with whitespace before and after the parantheses. We don't traditionally do that but the Checkstyle profile doesn't seem to catch that. I guess it would be safe to add that rule so we can fix those occurences. I would suggest the following as our next steps: 1. Clarify the thing with LineBreak*. 2. Decide (quickly, please) whether to remove the //CS comments or to allow them for now and optionally do something about them later. (I'm tending towards removing them but I don't have a problem if we do it the other way.) 3. Commit the patch to Trunk more or less as is (pending //CS decision). 3. Adjust the Checkstyle profile to allow "log" and disallow whitespace before and after parantheses. Then remove "log"-related //CS constants and excessive whitespace. 4. Merge the changes into the Temp_ComplexScripts parts. 5. Glenn could then provide a new patch against the branch which we could do a cursory review on. We apply that and experiment with what he's built. He can continue his work. 6. We continue to incrementally improve our coding standards. I'm happy to do the grunt work. Like Glenn, I don't like to hold principle discussions right now because that holds up several people from doing day-to-day work. That doesn't mean we can't hold them, but I don't see why we have to do it as a precondition to processing this patch. The patch gets us further but doesn't preclude any futher improvements later. Please, let's get this done. Jeremias Maerki