struts-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Frank W. Zammetti" <fzli...@omnytex.com>
Subject Re: SAF1 Checkstyle
Date Fri, 16 Jun 2006 05:04:05 GMT
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.

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: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Mime
View raw message