On 6/15/06, Frank W. Zammetti <[EMAIL PROTECTED]> wrote:
Ted Husted wrote: > You might at least want to start those discussions about the > checkstyle settings, so that we can develop a strategy about how they > would be fixed the next time there is a window of opportunity. Fair enough... I saw two issues that seem to account for a large number of the complaints... * The issue "Variable xxxx must be private and have accessor methods.". This is the VisibilityModifierCheck check. I don't think directly addressing it, i.e., following the suggestion it gives, is the right answer... I can't imagine it wouldn't break things, either in the SAF codebase itself or in users' application code. Fortunately, there appears to be a setting in Checkstyle to deal with this: http://checkstyle.sourceforge.net/config_design.html#VisibilityModifier Or we could remove this check entirely. I'd vote for just setting protectedAllowed to true though.
I wish there was a way to tell Checkstyle to allow protected variables for pre-existing code, but complain about new code. ;-) The fact is that we can't change protected variables to private in most places because it risks breaking any extending classes. We could fix the ones in Struts, of course, but we can't do anything about developers extending Struts in their own apps. I suppose we could add the getters and setters and deprecate the variable itself, making it private in a later release. For now, though, I agree that setting protectedAllowed to true is the most expedient option. -- Martin Cooper Looking at core alone, that would probably get rid of half of the
remaining issues. This is the only proposed rule change I have at this point. * The issue "Expected @throws tag for xxxx". This is coming up a lot because there are runtime exceptions thrown in methods that are not declared (which is of course valid, but doesn't follow the style rules). I would suggestion simply declaring them... I don't *think* that has any side-effects... anyone think there is? > When I did a a time study in February, curing the the current errors > with the current settings would take at least 40 hard hours. We need > to find a way to do the work faster, or get more people to work on it. Agreed, and your estimate may even be too optimistic given that first bullet above, if the rule change wasn't implemented. I frankly don't see another way to deal with that particular complaint. > (We probably should have made this a Google Summer of Code project.) Hehe, yeah :) > There is also the issue of how we want to handle the exception issues. > We will be addressing exception handling in SAF 2, so it's a pertinent > question. I certainly don't disagree that some exception handling updates should be looked at, but I'm not sure that's pertinent in the context of Checkstyle complaints... the exception bullet above I don't think should be expanded to modifying how exceptions are handled now, it's just getting rid of the complaints, which I think is as simple as declaring the exceptions. > We might also consider fixing some of the errors at a time, for > example maybe just the exception handlers. That could have less of an > impact that trying to cure all six thousand at once. Same point as the above paragraph... I think we're talking about two different things really. Although, if you wanted to tackle just one category of complaint at a time, that would be fine... I was thinking of a package at a time, but the work breakdown can certainly be done any of a number of ways. Ultimately though, that one rule change above would cut down a pretty substantial number of complaints, and in this case I don't think changing the rules is a cop-out. So, I'd like to see a consensus reached on that alone, everything else could be back-burnered as far as I'm concerned. > -Ted. Frank --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
