You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by Daniel Jeliński <dj...@gmail.com> on 2017/03/11 23:48:20 UTC
Dealing with issues reported by Findbugs
Hi all,
I started fixing code issues reported by Findbugs; right now it is
reporting 4000+ issues in lucene/solr repository. I could use some guidance:
1) Will one JIRA issue be sufficient to cover all Findbugs-related items,
or should I raise separate items for distinct problems reported by
Findbugs? I raised LUCENE-7739 as a catch-all issue, but I can split it if
that's preferred.
2) My plan is to fix trivial issues first, then work on the harder ones. I
already sent a patch to fix issues related to unnecessary boxing/unboxing
when parsing strings. That patch covers the entire codebase, but in my
opinion it's fairly straightforward. Is that acceptable, or should I split
the patch somehow? Like, lucene/solr, or one file at a time, or one issue
at a time or...
Ideas welcome.
Regards,
Daniel
Re: Dealing with issues reported by Findbugs
Posted by Daniel Jeliński <dj...@gmail.com>.
Hi Pushkar,
First off, thank you for your work on making Lucene and Solr better for
everyone!
I had a look at your patches; as you probably know, they are not
committable in their current form.
Your patches are adding comments about bugs found but not fixed, fixing
multiple bugs at a time, and making whitespace changes. Multiple bugs might
probably be acceptable, but make the patches harder to review. The other
issues are not acceptable.
I see you noticed that some of the problems reported by findbugs can be
fixed in a few ways - either by removing the broken code and keeping the
current behavior, or by modifying the code to match the author's intention
(where it's possible to guess). Modifying code is riskier, so involving the
original author in the review would make sense.
Other than that, having a patch to review is more likely to get attention
than pointing out a problem with unclear impact. Especially if you explain
in JIRA why you chose one way of fixing over the other and/or have tests to
demonstrate the problem and correctness of your fix.
With all that said, I'm not a committer on this project, so ultimately the
fate of your patches will not be decided by me.
Regards,
Daniel
PS. The test cases using thread.run instead of thread.start are likely
intentional; notice how the tests expect an exception coming out of run().
2017-03-15 23:48 GMT+01:00 Pushkar Raste <pu...@gmail.com>:
> Hi Daniel,
> I had opened SOLR-10080 a while back. I think Christine Porsche did pick
> up one small change from the, there are few things that still need to be
> looked over and address. I have added comments/questions in the patch when
> I had doubts. Patch addressed issues at solr package level
>
> I was working on another patch for Lucerne and you can find it at
> https://github.com/praste/lucene-solr/tree/findbugs-lucene?files=1
>
> Feel free to take a look at it.
>
> On Mar 11, 2017 6:22 PM, "Daniel Jeliński" <dj...@gmail.com> wrote:
>
>> Hi all,
>> I started fixing code issues reported by Findbugs; right now it is
>> reporting 4000+ issues in lucene/solr repository. I could use some guidance:
>> 1) Will one JIRA issue be sufficient to cover all Findbugs-related items,
>> or should I raise separate items for distinct problems reported by
>> Findbugs? I raised LUCENE-7739 as a catch-all issue, but I can split it if
>> that's preferred.
>> 2) My plan is to fix trivial issues first, then work on the harder ones.
>> I already sent a patch to fix issues related to unnecessary boxing/unboxing
>> when parsing strings. That patch covers the entire codebase, but in my
>> opinion it's fairly straightforward. Is that acceptable, or should I split
>> the patch somehow? Like, lucene/solr, or one file at a time, or one issue
>> at a time or...
>>
>> Ideas welcome.
>> Regards,
>> Daniel
>>
>
Re: Dealing with issues reported by Findbugs
Posted by Pushkar Raste <pu...@gmail.com>.
Hi Daniel,
I had opened SOLR-10080 a while back. I think Christine Porsche did pick
up one small change from the, there are few things that still need to be
looked over and address. I have added comments/questions in the patch when
I had doubts. Patch addressed issues at solr package level
I was working on another patch for Lucerne and you can find it at
https://github.com/praste/lucene-solr/tree/findbugs-lucene?files=1
Feel free to take a look at it.
On Mar 11, 2017 6:22 PM, "Daniel Jeliński" <dj...@gmail.com> wrote:
> Hi all,
> I started fixing code issues reported by Findbugs; right now it is
> reporting 4000+ issues in lucene/solr repository. I could use some guidance:
> 1) Will one JIRA issue be sufficient to cover all Findbugs-related items,
> or should I raise separate items for distinct problems reported by
> Findbugs? I raised LUCENE-7739 as a catch-all issue, but I can split it if
> that's preferred.
> 2) My plan is to fix trivial issues first, then work on the harder ones. I
> already sent a patch to fix issues related to unnecessary boxing/unboxing
> when parsing strings. That patch covers the entire codebase, but in my
> opinion it's fairly straightforward. Is that acceptable, or should I split
> the patch somehow? Like, lucene/solr, or one file at a time, or one issue
> at a time or...
>
> Ideas welcome.
> Regards,
> Daniel
>
Re: Dealing with issues reported by Findbugs
Posted by Daniel Jeliński <dj...@gmail.com>.
Hi Shawn,
For what it's worth, I'm using Idea FindBugs plugin. There is some overlap
between Idea warnings and Findbugs, but there's also a number of warnings
unique to each of them.
I'm starting with performance warnings, because the benefit of fixing them
is usually non-controversial. I don't plan to add any suppressions, because
it would require adding a compile-time dependency on Findbugs, and also
because I'm not a fan of hiding problems.
Regards,
Daniel
2017-03-12 21:53 GMT+01:00 Shawn Heisey <ap...@elyograg.org>:
> On 3/11/2017 4:48 PM, Daniel Jeliński wrote:
> > I started fixing code issues reported by Findbugs; right now it is
> > reporting 4000+ issues in lucene/solr repository. I could use some
> > guidance:
> > 1) Will one JIRA issue be sufficient to cover all Findbugs-related
> > items, or should I raise separate items for distinct problems reported
> > by Findbugs? I raised LUCENE-7739 as a catch-all issue, but I can
> > split it if that's preferred.
> > 2) My plan is to fix trivial issues first, then work on the harder
> > ones. I already sent a patch to fix issues related to unnecessary
> > boxing/unboxing when parsing strings. That patch covers the entire
> > codebase, but in my opinion it's fairly straightforward. Is that
> > acceptable, or should I split the patch somehow? Like, lucene/solr, or
> > one file at a time, or one issue at a time or...
>
> I wonder how much overlap there is between FindBugs and IDE warnings
> from Eclipse and Idea.
>
> I have wanted to tackle the thousands of warnings that Eclipse shows me
> for quite some time, but I don't really know how to tell when the
> warning should be taken seriously and when it should be suppressed.
> When I have mentioned it before, nobody else really has any interest,
> doesn't consider it to be a problem.
>
> Thanks,
> Shawn
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>
>
Re: Dealing with issues reported by Findbugs
Posted by Shawn Heisey <ap...@elyograg.org>.
On 3/11/2017 4:48 PM, Daniel Jeli\u0144ski wrote:
> I started fixing code issues reported by Findbugs; right now it is
> reporting 4000+ issues in lucene/solr repository. I could use some
> guidance:
> 1) Will one JIRA issue be sufficient to cover all Findbugs-related
> items, or should I raise separate items for distinct problems reported
> by Findbugs? I raised LUCENE-7739 as a catch-all issue, but I can
> split it if that's preferred.
> 2) My plan is to fix trivial issues first, then work on the harder
> ones. I already sent a patch to fix issues related to unnecessary
> boxing/unboxing when parsing strings. That patch covers the entire
> codebase, but in my opinion it's fairly straightforward. Is that
> acceptable, or should I split the patch somehow? Like, lucene/solr, or
> one file at a time, or one issue at a time or...
I wonder how much overlap there is between FindBugs and IDE warnings
from Eclipse and Idea.
I have wanted to tackle the thousands of warnings that Eclipse shows me
for quite some time, but I don't really know how to tell when the
warning should be taken seriously and when it should be suppressed.
When I have mentioned it before, nobody else really has any interest,
doesn't consider it to be a problem.
Thanks,
Shawn
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: Dealing with issues reported by Findbugs
Posted by Daniel Jeliński <dj...@gmail.com>.
Hi Adrien,
Thanks for your reply. I renamed the JIRA issue to address a specific
FindBugs problem, will raise more issues for other problems.
Regards,
Daniel
2017-03-12 18:31 GMT+01:00 Adrien Grand <jp...@gmail.com>:
> Hi Daniel,
>
> Thanks for helping! I'm currently away so I can't test your LUCENE-7739
> patch but at first sight it looks good. I think patches that cover the
> entire codebase are ok however we tend to have a 1-1 mapping between issues
> and patches so I think you should open new issues if you want to address
> new kinds of Findbugs-reported issues in the future. Does it make sense to
> you?
>
> Le dim. 12 mars 2017 à 03:22, Daniel Jeliński <dj...@gmail.com> a
> écrit :
>
>> Hi all,
>> I started fixing code issues reported by Findbugs; right now it is
>> reporting 4000+ issues in lucene/solr repository. I could use some guidance:
>> 1) Will one JIRA issue be sufficient to cover all Findbugs-related items,
>> or should I raise separate items for distinct problems reported by
>> Findbugs? I raised LUCENE-7739 as a catch-all issue, but I can split it if
>> that's preferred.
>> 2) My plan is to fix trivial issues first, then work on the harder ones.
>> I already sent a patch to fix issues related to unnecessary boxing/unboxing
>> when parsing strings. That patch covers the entire codebase, but in my
>> opinion it's fairly straightforward. Is that acceptable, or should I split
>> the patch somehow? Like, lucene/solr, or one file at a time, or one issue
>> at a time or...
>>
>> Ideas welcome.
>> Regards,
>> Daniel
>>
>
Re: Dealing with issues reported by Findbugs
Posted by Adrien Grand <jp...@gmail.com>.
Hi Daniel,
Thanks for helping! I'm currently away so I can't test your LUCENE-7739
patch but at first sight it looks good. I think patches that cover the
entire codebase are ok however we tend to have a 1-1 mapping between issues
and patches so I think you should open new issues if you want to address
new kinds of Findbugs-reported issues in the future. Does it make sense to
you?
Le dim. 12 mars 2017 à 03:22, Daniel Jeliński <dj...@gmail.com> a
écrit :
> Hi all,
> I started fixing code issues reported by Findbugs; right now it is
> reporting 4000+ issues in lucene/solr repository. I could use some guidance:
> 1) Will one JIRA issue be sufficient to cover all Findbugs-related items,
> or should I raise separate items for distinct problems reported by
> Findbugs? I raised LUCENE-7739 as a catch-all issue, but I can split it if
> that's preferred.
> 2) My plan is to fix trivial issues first, then work on the harder ones. I
> already sent a patch to fix issues related to unnecessary boxing/unboxing
> when parsing strings. That patch covers the entire codebase, but in my
> opinion it's fairly straightforward. Is that acceptable, or should I split
> the patch somehow? Like, lucene/solr, or one file at a time, or one issue
> at a time or...
>
> Ideas welcome.
> Regards,
> Daniel
>