You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2010/07/02 23:13:25 UTC
svn commit: r960104 -
/tomcat/trunk/test/org/apache/catalina/mbeans/TestRegistration.java
Author: markt
Date: Fri Jul 2 21:13:25 2010
New Revision: 960104
URL: http://svn.apache.org/viewvc?rev=960104&view=rev
Log:
A few more FindBugs issues
Modified:
tomcat/trunk/test/org/apache/catalina/mbeans/TestRegistration.java
Modified: tomcat/trunk/test/org/apache/catalina/mbeans/TestRegistration.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/mbeans/TestRegistration.java?rev=960104&r1=960103&r2=960104&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/catalina/mbeans/TestRegistration.java (original)
+++ tomcat/trunk/test/org/apache/catalina/mbeans/TestRegistration.java Fri Jul 2 21:13:25 2010
@@ -46,7 +46,10 @@ public class TestRegistration extends To
final Tomcat tomcat = getTomcatInstance();
final File contextDir = new File(getTemporaryDirectory(), "webappFoo");
- contextDir.mkdir();
+ if (!contextDir.exists()) {
+ if (!contextDir.mkdir())
+ fail("Failed to create: [" + contextDir.toString() + "]");
+ }
tomcat.addContext("/foo", contextDir.getAbsolutePath());
tomcat.start();
@@ -70,7 +73,10 @@ public class TestRegistration extends To
tomcat.getEngine().addChild(host);
final File contextDir2 = new File(getTemporaryDirectory(), "webappFoo2");
- contextDir2.mkdir();
+ if (!contextDir2.exists()) {
+ if (!contextDir2.mkdir())
+ fail("Failed to create: [" + contextDir2.toString() + "]");
+ }
tomcat.addContext(host, "/foo2", contextDir2.getAbsolutePath());
tomcat.start();
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r960104 - /tomcat/trunk/test/org/apache/catalina/mbeans/TestRegistration.java
Posted by Rainer Jung <ra...@kippdata.de>.
On 08.07.2010 10:30, Marc Guillemot wrote:
> Mark Thomas wrote:
>> ...
>>> JavadocType (versionFormat) -> currently 539 failures
>>
>> Not sure what exactly you are proposing here. Could you clarify?
>
> I mean the format of the @Version JavaDoc tag what Konstantin Kolinko
> has started to fix:
> http://tomcat.markmail.org/thread/bfry4x5gwbhezo6x
>
>> ...
>>> Additionally it seems that you pay attention at some warnings emitted by
>>> your IDE. It would be interesting to express these rules as checkstyle
>>> (or whatever) rule that can be checked automatically. Can you list the
>>> Eclipse style warnings you try to fix?
>>
>> Just the standard Java compiler warnings.
>
> is there an agreement among the committers for that? ;-)
>
> I would be cautious with Eclipse warnings. Some of them don't make sense
> (or at least not alone like "Serializable class without
> serialVersionUID") and others have pros and cons (like removing the
> "else" when the "if" ends with a return)
>
>>
>>> I agree that coherent code style is only cosmetic in a first time as it
>>> doesn't change the resulting byte code. Nevertheless it makes easier to
>>> concentrate on the important things: the code content, without being
>>> distracted by the form. Therefore it helps to find real problems and
>>> make easier to compare files, apply patches, ...
>>
>> But it makes it harder to back-port changes. I'm not against cleaning up
>> the code, we just need to be aware of the consequences of doing so.
>
> Two solutions for easy back ports
> 1) never make change, neither yet, nor in the future and keep
> everything, even if dirty
> 2) apply clean style rules on all maintained code versions
>
> Of course I prefer (2) ;-)
Not so sure. Often code history is important and you compare older code
revisions to new ones. Big style changes or cleanup makes this hard.
I'm +1 to style cleanup in general, because there are quite some old
files that are ugly and hard to read. But I wonder whether we are maybe
now coming closer to the point, where we will only backport bug fixes
from TC 7 to 6 and 5.5. My impression is, that we did quite some feature
backporting the last months and 6 should really reach feature maturity
now at patch level 28.
If so, it could make sense to only clean up the TC 7 code base.
Regards,
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r960104 - /tomcat/trunk/test/org/apache/catalina/mbeans/TestRegistration.java
Posted by Marc Guillemot <mg...@yahoo.fr>.
Mark Thomas wrote:
> ...
>> JavadocType (versionFormat) -> currently 539 failures
>
> Not sure what exactly you are proposing here. Could you clarify?
I mean the format of the @Version JavaDoc tag what Konstantin Kolinko
has started to fix:
http://tomcat.markmail.org/thread/bfry4x5gwbhezo6x
> ...
>> Additionally it seems that you pay attention at some warnings emitted by
>> your IDE. It would be interesting to express these rules as checkstyle
>> (or whatever) rule that can be checked automatically. Can you list the
>> Eclipse style warnings you try to fix?
>
> Just the standard Java compiler warnings.
is there an agreement among the committers for that? ;-)
I would be cautious with Eclipse warnings. Some of them don't make sense
(or at least not alone like "Serializable class without
serialVersionUID") and others have pros and cons (like removing the
"else" when the "if" ends with a return)
>
>> I agree that coherent code style is only cosmetic in a first time as it
>> doesn't change the resulting byte code. Nevertheless it makes easier to
>> concentrate on the important things: the code content, without being
>> distracted by the form. Therefore it helps to find real problems and
>> make easier to compare files, apply patches, ...
>
> But it makes it harder to back-port changes. I'm not against cleaning up
> the code, we just need to be aware of the consequences of doing so.
Two solutions for easy back ports
1) never make change, neither yet, nor in the future and keep
everything, even if dirty
2) apply clean style rules on all maintained code versions
Of course I prefer (2) ;-)
Cheers,
Marc.
--
Blog: http://mguillem.wordpress.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r960104 - /tomcat/trunk/test/org/apache/catalina/mbeans/TestRegistration.java
Posted by Mark Thomas <ma...@apache.org>.
On 08/07/2010 06:37, Marc Guillemot wrote:
> Hi Mark,
>
> Mark Thomas wrote:
>> ...
>> There is nothing stopping you running FindBugs, Checkstyle or any other
>> tool locally. I use FindBugs on the Tomact 7 code-base all the time.
>
> what about sharing the config and including it into the build?
It is just the default configuration for the Eclipse plug-in, not part
of the build script or anything.
> According to the negative comments on commits, everybody should agree on
> following checks
> FileTabCharacter -> currently 146 failures
+1
> JavadocType (versionFormat) -> currently 539 failures
Not sure what exactly you are proposing here. Could you clarify?
> Following simple steps could be:
Seems reasonable, providing there is agreement amongst the committers
for each additional check that is added.
> Additionally it seems that you pay attention at some warnings emitted by
> your IDE. It would be interesting to express these rules as checkstyle
> (or whatever) rule that can be checked automatically. Can you list the
> Eclipse style warnings you try to fix?
Just the standard Java compiler warnings.
> I agree that coherent code style is only cosmetic in a first time as it
> doesn't change the resulting byte code. Nevertheless it makes easier to
> concentrate on the important things: the code content, without being
> distracted by the form. Therefore it helps to find real problems and
> make easier to compare files, apply patches, ...
But it makes it harder to back-port changes. I'm not against cleaning up
the code, we just need to be aware of the consequences of doing so.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r960104 - /tomcat/trunk/test/org/apache/catalina/mbeans/TestRegistration.java
Posted by Peter Roßbach <pr...@objektpark.de>.
I totally argreed with Marc Guillemot.
It is time to more invest to the cosmetic things at the tomcat code
basis.
Peter
Am 08.07.2010 um 07:37 schrieb Marc Guillemot:
> Hi Mark,
>
> Mark Thomas wrote:
>> ...
>> There is nothing stopping you running FindBugs, Checkstyle or any
>> other
>> tool locally. I use FindBugs on the Tomact 7 code-base all the time.
>
> what about sharing the config and including it into the build?
>
>>> What does the other developers think?
>> +0 providing we start with a *very* limited set of checks. There
>> should
>> be very clear agreement amongst a significant majority of the
>> committers
>> to add additional checks.
>> I'd like to see a proposed list of checks before this starts being
>> used
>> on any of the Tomcat code bases. I'd suggest a single check for
>> each of
>> Checkstyle and Findbugs to start.
>
> The checks make sense if they are accepted by the majority of the
> committers as they should be integral part of the build. This is the
> reason why I've only proposed one check in patch 49268.
>
> According to the negative comments on commits, everybody should
> agree on following checks
> FileTabCharacter -> currently 146 failures
> JavadocType (versionFormat) -> currently 539 failures
>
> Following simple steps could be:
> RedundantImport -> 11 failures
> UnusedImports -> 53 failures
> ModifierOrder -> 211 failures
> RedundantModifier -> 1377 failures (seems that nobody knows the
> default visibility rule of a public interface)
> LeftCurly -> 498 failures
> EqualsHashCode -> 5 failures
> FinalParameters -> 12763 failures
> FinalLocalVariable -> 8201 failures (not so much Java developers
> really know how the power of the final keyword)
>
> Additionally it seems that you pay attention at some warnings
> emitted by your IDE. It would be interesting to express these rules
> as checkstyle (or whatever) rule that can be checked automatically.
> Can you list the Eclipse style warnings you try to fix?
>
> I agree that coherent code style is only cosmetic in a first time as
> it doesn't change the resulting byte code. Nevertheless it makes
> easier to concentrate on the important things: the code content,
> without being distracted by the form. Therefore it helps to find
> real problems and make easier to compare files, apply patches, ...
>
> Cheers,
> Marc.
> --
> Blog: http://mguillem.wordpress.com
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r960104 - /tomcat/trunk/test/org/apache/catalina/mbeans/TestRegistration.java
Posted by Marc Guillemot <mg...@yahoo.fr>.
Hi Mark,
Mark Thomas wrote:
> ...
> There is nothing stopping you running FindBugs, Checkstyle or any other
> tool locally. I use FindBugs on the Tomact 7 code-base all the time.
what about sharing the config and including it into the build?
>
>> What does the other developers think?
>
> +0 providing we start with a *very* limited set of checks. There should
> be very clear agreement amongst a significant majority of the committers
> to add additional checks.
>
> I'd like to see a proposed list of checks before this starts being used
> on any of the Tomcat code bases. I'd suggest a single check for each of
> Checkstyle and Findbugs to start.
The checks make sense if they are accepted by the majority of the
committers as they should be integral part of the build. This is the
reason why I've only proposed one check in patch 49268.
According to the negative comments on commits, everybody should agree on
following checks
FileTabCharacter -> currently 146 failures
JavadocType (versionFormat) -> currently 539 failures
Following simple steps could be:
RedundantImport -> 11 failures
UnusedImports -> 53 failures
ModifierOrder -> 211 failures
RedundantModifier -> 1377 failures (seems that nobody knows the default
visibility rule of a public interface)
LeftCurly -> 498 failures
EqualsHashCode -> 5 failures
FinalParameters -> 12763 failures
FinalLocalVariable -> 8201 failures (not so much Java developers really
know how the power of the final keyword)
Additionally it seems that you pay attention at some warnings emitted by
your IDE. It would be interesting to express these rules as checkstyle
(or whatever) rule that can be checked automatically. Can you list the
Eclipse style warnings you try to fix?
I agree that coherent code style is only cosmetic in a first time as it
doesn't change the resulting byte code. Nevertheless it makes easier to
concentrate on the important things: the code content, without being
distracted by the form. Therefore it helps to find real problems and
make easier to compare files, apply patches, ...
Cheers,
Marc.
--
Blog: http://mguillem.wordpress.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r960104 - /tomcat/trunk/test/org/apache/catalina/mbeans/TestRegistration.java
Posted by Mark Thomas <ma...@apache.org>.
On 07/07/2010 17:53, Peter Roßbach wrote:
> Then I can check my personal problem with the tab/space replacement,
> before checkin code :-)
> Currently we have in 144 files a tab instead a space replacement issue.
There is nothing stopping you running FindBugs, Checkstyle or any other
tool locally. I use FindBugs on the Tomact 7 code-base all the time.
> Ok, I want to fix it.
>
> Later we can add more checkstyles. Who wants to help, me?
>
> In the last two years we made a lot of source code cosmetic changes and
> these help to get a more readable codebase.
Agreed the code-base is more readable.
> I think that Marc Guillemot can help us to get more code and test
> quality to the project.
My impression is that most of these changes are cosmetic rather than
code quality issues. I don't think fixing all the current FindBugs,
Checkstyle and Eclipse warnings will result in a significant increase in
code quality. It would, however, make it easier to spot when errors do
creep in.
> What does the other developers think?
+0 providing we start with a *very* limited set of checks. There should
be very clear agreement amongst a significant majority of the committers
to add additional checks.
I'd like to see a proposed list of checks before this starts being used
on any of the Tomcat code bases. I'd suggest a single check for each of
Checkstyle and Findbugs to start.
I actually think we'd get more value out of the unused code detector.
Going through the 7.0.x codebase and removing unused code is still on my
todo list. I suspect there is a reasonable amount we can simply delete.
Mark
>
> Regards
> Peter
>
> Am 05.07.2010 um 11:34 schrieb Mark Thomas:
>
>> On 05/07/2010 09:27, Marc Guillemot wrote:
>>> markt@apache.org wrote:
>>>> Author: markt
>>>> Date: Fri Jul 2 21:13:25 2010
>>>> New Revision: 960104
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=960104&view=rev
>>>> Log:
>>>> A few more FindBugs issues
>>> > ...
>>>
>>> why don't you integrate checkstyle (and FindBugs) in the build?
>>
>> The Tomcat code doesn't follow any standards consistently so the
>> number of warnings that would get reported is huge. As far as I am
>> aware the actual problems have been fixed. The remaining issues are
>> cosmetic. There may be some real issues hiding in there somewhere but
>> if there are, they aren't things that users are reporting as problems
>> else there would be a bugzilla entry for it.
>>
>> I don't think (I could be wrong) there is much interest in investing a
>> lot of effort in a bug code clean-up. Gradual improvement seems to be
>> the preferred approach at the moment.
>>
>>> It's the only safe way to ensure quality
>>
>> I don't think it is as black and white as that. These tools have their
>> place and I think the Tomcat project has used them appropriately.
>> FindBugs, for example, still reports 1000s of issues but the actual
>> problems were fixed quite some time ago. The cosmetic issues were not.
>>
>> There is a valid argument that fixing the cosmetic issues does more
>> harm than good by making it harder to do diffs between major versions.
>>
>>> and to avoid useless style discussions.
>>
>> I don't recall any useless style discussions. I don't think this is an
>> issue we need to solve.
>>
>> Mark
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r960104 - /tomcat/trunk/test/org/apache/catalina/mbeans/TestRegistration.java
Posted by Peter Roßbach <pr...@objektpark.de>.
Hi!
I miss the checkstyle and findbugs quality checks in the project.
Marc has contributed a good starting point at
https://issues.apache.org/bugzilla/show_bug.cgi?id=49268
Then I can check my personal problem with the tab/space replacement,
before checkin code :-)
Currently we have in 144 files a tab instead a space replacement
issue. Ok, I want to fix it.
Later we can add more checkstyles. Who wants to help, me?
In the last two years we made a lot of source code cosmetic changes
and these help to get a more readable codebase.
I think that Marc Guillemot can help us to get more code and test
quality to the project.
What does the other developers think?
Regards
Peter
Am 05.07.2010 um 11:34 schrieb Mark Thomas:
> On 05/07/2010 09:27, Marc Guillemot wrote:
>> markt@apache.org wrote:
>>> Author: markt
>>> Date: Fri Jul 2 21:13:25 2010
>>> New Revision: 960104
>>>
>>> URL: http://svn.apache.org/viewvc?rev=960104&view=rev
>>> Log:
>>> A few more FindBugs issues
>> > ...
>>
>> why don't you integrate checkstyle (and FindBugs) in the build?
>
> The Tomcat code doesn't follow any standards consistently so the
> number of warnings that would get reported is huge. As far as I am
> aware the actual problems have been fixed. The remaining issues are
> cosmetic. There may be some real issues hiding in there somewhere
> but if there are, they aren't things that users are reporting as
> problems else there would be a bugzilla entry for it.
>
> I don't think (I could be wrong) there is much interest in investing
> a lot of effort in a bug code clean-up. Gradual improvement seems to
> be the preferred approach at the moment.
>
>> It's the only safe way to ensure quality
>
> I don't think it is as black and white as that. These tools have
> their place and I think the Tomcat project has used them
> appropriately. FindBugs, for example, still reports 1000s of issues
> but the actual problems were fixed quite some time ago. The cosmetic
> issues were not.
>
> There is a valid argument that fixing the cosmetic issues does more
> harm than good by making it harder to do diffs between major versions.
>
>> and to avoid useless style discussions.
>
> I don't recall any useless style discussions. I don't think this is
> an issue we need to solve.
>
> Mark
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: svn commit: r960104 - /tomcat/trunk/test/org/apache/catalina/mbeans/TestRegistration.java
Posted by Mark Thomas <ma...@apache.org>.
On 05/07/2010 09:27, Marc Guillemot wrote:
> markt@apache.org wrote:
>> Author: markt
>> Date: Fri Jul 2 21:13:25 2010
>> New Revision: 960104
>>
>> URL: http://svn.apache.org/viewvc?rev=960104&view=rev
>> Log:
>> A few more FindBugs issues
> > ...
>
> why don't you integrate checkstyle (and FindBugs) in the build?
The Tomcat code doesn't follow any standards consistently so the number
of warnings that would get reported is huge. As far as I am aware the
actual problems have been fixed. The remaining issues are cosmetic.
There may be some real issues hiding in there somewhere but if there
are, they aren't things that users are reporting as problems else there
would be a bugzilla entry for it.
I don't think (I could be wrong) there is much interest in investing a
lot of effort in a bug code clean-up. Gradual improvement seems to be
the preferred approach at the moment.
> It's the only safe way to ensure quality
I don't think it is as black and white as that. These tools have their
place and I think the Tomcat project has used them appropriately.
FindBugs, for example, still reports 1000s of issues but the actual
problems were fixed quite some time ago. The cosmetic issues were not.
There is a valid argument that fixing the cosmetic issues does more harm
than good by making it harder to do diffs between major versions.
> and to avoid useless style discussions.
I don't recall any useless style discussions. I don't think this is an
issue we need to solve.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r960104 - /tomcat/trunk/test/org/apache/catalina/mbeans/TestRegistration.java
Posted by Marc Guillemot <mg...@yahoo.fr>.
markt@apache.org wrote:
> Author: markt
> Date: Fri Jul 2 21:13:25 2010
> New Revision: 960104
>
> URL: http://svn.apache.org/viewvc?rev=960104&view=rev
> Log:
> A few more FindBugs issues
> ...
why don't you integrate checkstyle (and FindBugs) in the build? It's the
only safe way to ensure quality and to avoid useless style discussions.
Or is the number of Tomcat commits still a marketing argument for
SpringSource? ;-)
Cheers,
Marc.
--
Blog: http://mguillem.wordpress.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org