You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by Ted Husted <te...@gmail.com> on 2006/01/22 12:59:58 UTC
Re: svn commit: r370938 [1/50] - in /struts: action/trunk/ action/trunk/conf/java/ action/trunk/src/java/org/apache/struts/ action/trunk/src/java/org/apache/struts/action/ action/trunk/src/java/org/apache/struts/chain/ action/trunk/src/java/org/apach
On 1/20/06, Niall Pemberton <ni...@blueyonder.co.uk> wrote:
> I haven't time to look through all 50 commit messages for this change - but
> a quick look at a few of the messages already brought up a couple of issues:
>
> * The reformatting of the svn keyswords @version $Rev$ $Date$ has moved the
> $ sign from the end of Date to the next line - effectively disabling the
> keyword and committing the expanded text - saw this on FormBeanConfig and
> FormPropertyConfig - assume its happened everywhere.
When the macro expands, it can exceed 80 characters per line, which
conflicts with the CheckStyle settings. To use the SVN macro, we
should increase the standard line length to at least 85 characters.
When this setting is changed, the line is automattically rewrapped, so
that it looks like this
* @version $Rev: 371073 $ $Date: 2005-08-26 21:58:39 -0400 (Fri, 26 Aug 2005) $
> * The class javadoc for ActionServlet - has merged all the list items (i.e.
> <li>....</li>) into one big paragraph - same things happended in
> FormPropertyConfig on the javadoc for the initial() method. I guess anywhere
> we've used lists in the javadoc, a similar thing has happened.
A related problem is that some tags that we have used, like <code>,
are being excused from the line count by IDEA, but counted by
Checkstyle. As a result, lines that use <code> may exceed 80
characters, even after automatic reformatting, and continue to
generate Checkstyle errors.
As to the markup for the list, one fix is to put blank lines around
the list, like this:
*
* <ul>
*
* <li>Instance and static variables MUST NOT be used to store
information related to
* the state of a particular request. They MAY be used to share global resources
* across requests for the same action.</li>
*
* <li>Access to other resources (JavaBeans, session variables, etc.) MUST be
* synchronized if those resources require protection. (Generally,
however, resource
* classes should be designed to provide their own protection where
necessary.</li>
*
* </ul>
*
Which, I'd be happy to do.
> Alot of the changes seem trivial and unnecessary - much of the reformatting
> IMO doesn't improve anything. Where it has made major changes is to
> re-organise the order of variables/methods which is going to make it more
> difficult to look back and find out when/if something changed. The other
> thing is that this is such a big change there is no way to check it all out
> and be sure that it hasn't screwed up in other ways, other than just
> formatting. Before seeing the effects of automated re-formatting, I was
> neutral - now I'm pretty sure I don't like it. Something passive like
> checkstyle is fine, but this direction seems like bad news to me.
I think the truly bad news is that, throughout the project, we have
more than 23,395 checkstyle errors :(
Yes, 23,395+
Of course, nearly all of these errors are trivial and non-functional.
But, the trival errors show up on the Checkstyle reports too, so that
it is difficult or impossible to see the forrest for the trees. With
twenty thousand errors blocking our view, the Checkstyle reports have
little or no practical value.
If we are going to use Checkstyle, then we should use it, and do
whatever we can to bring the project back in line with the Checkstyle
standards. This will cause some pain in the short run, but it is pain
that we brought upon ourselves through inattention.
If we are going to let all twenty thousand errors stand, then perhaps
we should supress the Checkstyle report from the Classic subprojects,
and endeavor to do better on the newer subprojects.
-Ted.
>
> Niall
>
> ----- Original Message -----
> From: <hu...@apache.org>
> To: <co...@struts.apache.org>
> Sent: Saturday, January 21, 2006 12:21 AM
> Subject: svn commit: r370938 [1/50] - in /struts: action/trunk/
> action/trunk/conf/java/ action/trunk/src/java/org/apache/struts/
> action/trunk/src/java/org/apache/struts/action/
> action/trunk/src/java/org/apache/struts/chain/
> action/trunk/src/java/org/apache/stru...
>
>
> > Author: husted
> > Date: Fri Jan 20 16:19:02 2006
> > New Revision: 370938
> >
> > URL: http://svn.apache.org/viewcvs?rev=370938&view=rev
> > Log:
> > CheckStyle pass
> > * Reformat with Jalopy and IDEA
> > * No code or content changes
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
--
HTH, Ted.
http://www.husted.com/poe/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: svn commit: r370938 [1/50] - in /struts: action/trunk/ action/trunk/conf/java/ action/trunk/src/java/org/apache/struts/ action/trunk/src/java/org/apache/struts/action/ action/trunk/src/java/org/apache/struts/chain/ action/trunk/src/java/org/apach
Posted by Ted Husted <te...@gmail.com>.
On 1/22/06, Martin Cooper <ma...@apache.org> wrote:
> > When the macro expands, it can exceed 80 characters per line, which
> > conflicts with the CheckStyle settings. To use the SVN macro, we
> > should increase the standard line length to at least 85 characters.
>
> That's not necessary. Checkstyle can be configured to ignore the lines that
> have the SVN keywords on them. I thought I'd done this for Struts, but it
> seems I only did it for FileUpload. I'll fix that.
The problem is that IDEA is wrapping the column at 80, whether it's a
SVN keyword line or not.
-T.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: svn commit: r370938 [1/50] - in /struts: action/trunk/ action/trunk/conf/java/ action/trunk/src/java/org/apache/struts/ action/trunk/src/java/org/apache/struts/action/ action/trunk/src/java/org/apache/struts/chain/ action/trunk/src/java/org/apach
Posted by Martin Cooper <ma...@apache.org>.
On 1/22/06, Ted Husted <te...@gmail.com> wrote:
>
> On 1/20/06, Niall Pemberton <ni...@blueyonder.co.uk> wrote:
> > I haven't time to look through all 50 commit messages for this change -
> but
> > a quick look at a few of the messages already brought up a couple of
> issues:
> >
> > * The reformatting of the svn keyswords @version $Rev$ $Date$ has moved
> the
> > $ sign from the end of Date to the next line - effectively disabling the
> > keyword and committing the expanded text - saw this on FormBeanConfig
> and
> > FormPropertyConfig - assume its happened everywhere.
>
> When the macro expands, it can exceed 80 characters per line, which
> conflicts with the CheckStyle settings. To use the SVN macro, we
> should increase the standard line length to at least 85 characters.
That's not necessary. Checkstyle can be configured to ignore the lines that
have the SVN keywords on them. I thought I'd done this for Struts, but it
seems I only did it for FileUpload. I'll fix that.
--
Martin Cooper
When this setting is changed, the line is automattically rewrapped, so
> that it looks like this
>
> * @version $Rev: 371073 $ $Date: 2005-08-26 21:58:39 -0400 (Fri, 26 Aug
> 2005) $
>
>
> > * The class javadoc for ActionServlet - has merged all the list items (
> i.e.
> > <li>....</li>) into one big paragraph - same things happended in
> > FormPropertyConfig on the javadoc for the initial() method. I guess
> anywhere
> > we've used lists in the javadoc, a similar thing has happened.
>
> A related problem is that some tags that we have used, like <code>,
> are being excused from the line count by IDEA, but counted by
> Checkstyle. As a result, lines that use <code> may exceed 80
> characters, even after automatic reformatting, and continue to
> generate Checkstyle errors.
>
> As to the markup for the list, one fix is to put blank lines around
> the list, like this:
>
> *
> * <ul>
> *
> * <li>Instance and static variables MUST NOT be used to store
> information related to
> * the state of a particular request. They MAY be used to share global
> resources
> * across requests for the same action.</li>
> *
> * <li>Access to other resources (JavaBeans, session variables, etc.) MUST
> be
> * synchronized if those resources require protection. (Generally,
> however, resource
> * classes should be designed to provide their own protection where
> necessary.</li>
> *
> * </ul>
> *
>
> Which, I'd be happy to do.
>
> > Alot of the changes seem trivial and unnecessary - much of the
> reformatting
> > IMO doesn't improve anything. Where it has made major changes is to
> > re-organise the order of variables/methods which is going to make it
> more
> > difficult to look back and find out when/if something changed. The other
> > thing is that this is such a big change there is no way to check it all
> out
> > and be sure that it hasn't screwed up in other ways, other than just
> > formatting. Before seeing the effects of automated re-formatting, I was
> > neutral - now I'm pretty sure I don't like it. Something passive like
> > checkstyle is fine, but this direction seems like bad news to me.
>
> I think the truly bad news is that, throughout the project, we have
> more than 23,395 checkstyle errors :(
>
> Yes, 23,395+
>
> Of course, nearly all of these errors are trivial and non-functional.
> But, the trival errors show up on the Checkstyle reports too, so that
> it is difficult or impossible to see the forrest for the trees. With
> twenty thousand errors blocking our view, the Checkstyle reports have
> little or no practical value.
>
> If we are going to use Checkstyle, then we should use it, and do
> whatever we can to bring the project back in line with the Checkstyle
> standards. This will cause some pain in the short run, but it is pain
> that we brought upon ourselves through inattention.
>
> If we are going to let all twenty thousand errors stand, then perhaps
> we should supress the Checkstyle report from the Classic subprojects,
> and endeavor to do better on the newer subprojects.
>
> -Ted.
>
> >
> > Niall
> >
> > ----- Original Message -----
> > From: <hu...@apache.org>
> > To: <co...@struts.apache.org>
> > Sent: Saturday, January 21, 2006 12:21 AM
> > Subject: svn commit: r370938 [1/50] - in /struts: action/trunk/
> > action/trunk/conf/java/ action/trunk/src/java/org/apache/struts/
> > action/trunk/src/java/org/apache/struts/action/
> > action/trunk/src/java/org/apache/struts/chain/
> > action/trunk/src/java/org/apache/stru...
> >
> >
> > > Author: husted
> > > Date: Fri Jan 20 16:19:02 2006
> > > New Revision: 370938
> > >
> > > URL: http://svn.apache.org/viewcvs?rev=370938&view=rev
> > > Log:
> > > CheckStyle pass
> > > * Reformat with Jalopy and IDEA
> > > * No code or content changes
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> > For additional commands, e-mail: dev-help@struts.apache.org
> >
> >
>
>
> --
> HTH, Ted.
> http://www.husted.com/poe/
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>