Hi, Jeremias Maerki wrote: > 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".
(This is precisely why I suggested that we agree on an improved Checkstyle first, to avoid introducing unnecessary //CS comments.) I don’t really have an opinion about that. Since zero-warning won’t be achieved in the short term anyway, I suppose we could remove them for now. Once we decide to enforce a zero-warning policy then they will probably have to be used, along with a TODO warning indicated that this is old code that needs refactoring; and thus make the difference with new CSOK comments introduced later on with due care. > 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. I’m ok with that. Some less generic comments will need to be double-checked. Not that I don’t trust Glenn on that matter, but some parts of the code (especially layout) are tricky and it’s very easy to be mistaken. And I think a wrong Javadoc comment does more harm than no comment at all. > 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. +1 > 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.) +1 for removing them for now. > 3. Commit the patch to Trunk more or less as is (pending //CS decision). -1, among other things there are deprecated flags/methods that were removed and I feel that that must be discussed first (mainly Graphics2DAdapter.paintImage). > 3. Adjust the Checkstyle profile to allow "log" and disallow whitespace > before and after parantheses. Then remove "log"-related //CS constants > and excessive whitespace. +0, I would just put log in uppercase but I don’t really mind. > 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. I’m not happy with that approach. When this topic was first mentioned [1] I did say that the Checkstyle file needed improvement and that until then this would be premature to work on that. My advice was not followed and now we should apply this patch ASAP without discussion? I’m not sure that trying to force things is a good way to get involved. The consensus-based approach inherent to any Apache project is not being followed here. [1] http://markmail.org/message/bmbocjhmav3dlahc So now this patch is there, and it contains a lot of good stuff, and that’s great. But like I explained in my earlier message many things will have to be re-visited once a new Checkstyle file is put in place. So in the end this was a waste of time to do that now. I’ll list in a separate message the modifications that I feel need to be discussed before being applied (although I’ll be offline from tomorrow to Tuesday). For the rest, Jeremias, if you’re happy to apply them, feel free to proceed. Vincent
