Mailing-List: contact nutch-dev-help@lucene.apache.org; run by ezmlm
Precedence: bulk
Reply-To: nutch-dev@lucene.apache.org
Received-SPF: pass (asf.osuosl.org: local policy)
Message-ID: <44360ED6.10808@cs.put.poznan.pl>
Date: Fri, 07 Apr 2006 09:03:50 +0200
From: Dawid Weiss <dawid.weiss@cs.put.poznan.pl>
User-Agent: Thunderbird 1.5 (Windows/20051201)
MIME-Version: 1.0
To: nutch-dev@lucene.apache.org
Subject: Re: PMD integration
References: <44322C29.7090604@cs.put.poznan.pl>
 <4432AB2F.6050509@apache.org>	 <44336799.1090508@cs.put.poznan.pl>
 <443407E2.9000005@apache.org>	 <4434154F.1030700@cs.put.poznan.pl>
	 <4434289E.9030200@cs.put.poznan.pl>
 <44342E32.5000006@apache.org>	 <4434BD5F.3080108@cs.put.poznan.pl>
 <18f3c2ad0604060012u3af568bbw3dd06880627209bc@mail.gmail.com>
 <4434DAB6.7020600@cs.put.poznan.pl> <44356AEE.6030907@gmail.com>
In-Reply-To: <44356AEE.6030907@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


Hi Piotr,

 > that right now it is checking only main code (without plugins?).

Yes, that's correct -- I forgot to mention that. PMD target is hooked up 
with tests and stops the build if something fails. I thought the core 
code should be this strict; for plugins we can have more relaxed rules 
(in another target or even in the same one).

That's again up to you guys.

Dawid

P.S. Tom Copeland has already fixed the bug I mentioned in the patch. 
Quite impressive bugfix turnaround, isn't it. :)


Piotr Kosiorowski wrote:
> P.
> 
> 
> Dawid Weiss wrote:
>>
>> All right, I though I'd give it a go since I have a spare few minutes. 
>> Jura is off, so I made the patches available here --
>>
>> http://ophelia.cs.put.poznan.pl/~dweiss/nutch/
>>
>> pmd.patch is the build file patch and libraries (binaries are in a 
>> separate zip file pmd-ext.zip).
>>
>> pmd-fixes.patch fixes the current core code to go through pmd 
>> smoothly. I removed obvious unused code, but left FIXME comments where 
>> I wasn't sure if the removal can cause side effects (in these places 
>> PMD warnings are suppressed with NOPMD comments).
>>
>> I also discovered a bug in PMD... eh... nothing's perfect.
>>
>> https://sourceforge.net/tracker/?func=detail&atid=479921&aid=1465574&group_id=56262 
>>
>>
>> D.
>>
>>
>> Piotr Kosiorowski wrote:
>>> +1 - I offer my help - we can coordinate it and I can do a part of 
>>> work. I
>>> will also try to commit your patches quickly.
>>> Piotr
>>>
>>> On 4/6/06, Dawid Weiss <dawid.weiss@cs.put.poznan.pl> wrote:
>>>>
>>>>> Other options (raised on the Hadoop list) are Checkstyle:
>>>> PMD seems to be the best choice for an Apache project and they all seem
>>>> to perform at a similar level.
>>>>
>>>>> Anything that generates a lot of false positives is bad: it either
>>>>> causes us to skip analysis of lots of files, or ignore the warnings.
>>>>> Skipping the JavaCC-generated classes is reasonable, but I'm wary of
>>>>> skipping much else.
>>>> I thought a bit about this. The warnings PMD may actually make sense to
>>>> fix. Take a look at maxDoc here:
>>>>
>>>> class LuceneQueryOptimizer {
>>>>
>>>>    private static class LimitExceeded extends RuntimeException {
>>>>      private int maxDoc;
>>>>      public LimitExceeded(int maxDoc) { this.maxDoc = maxDoc; }
>>>>    }
>>>> ...
>>>>
>>>> maxDoc is accessed from LuceneQueryOptimizer which requires a synthetic
>>>> accessor in LimitExceeded. It also may look confusing because you
>>>> declare a field private to a class, but use it from the outside...
>>>> changing declarations to something like this:
>>>>
>>>> class LuceneQueryOptimizer {
>>>>
>>>>    private static class LimitExceeded extends RuntimeException {
>>>>      final int maxDoc;
>>>>      public LimitExceeded(int maxDoc) { this.maxDoc = maxDoc; }
>>>>    }
>>>> ...
>>>>
>>>> removes the warning and also seems to make more sense (note that 
>>>> package
>>>> scope of maxDoc doesn't really expose it much more than before because
>>>> the entire class is private).
>>>>
>>>> So... if you agree to change existing warnings as shown above (there's
>>>> not that many) then integrating PMD with a set of sensible rules may
>>>> help detecting bad smells in the future (I couldn't resist -- it really
>>>> is called like this in software engineering :). I only used dead code
>>>> detection ruleset for now, other rulesets can be checked and we will 
>>>> see
>>>> if they help or quite the contrary.
>>>>
>>>> If developers agree to the above I'll create a patch together with what
>>>> needs to be fixed to cleanly compile. Otherwise I see little sense in
>>>> integrating PMD.
>>>>
>>>> D.
>>>>
>>>>
>>>>
>>>>
>>>
>>
>