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 <pushkar.ra...@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" <djelins...@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 >> >
