On Fri, Nov 1, 2013 at 10:50 AM, Vincent Hennebert <[email protected]>wrote:
> On 01/11/13 18:21, Glenn Adams wrote: > >> On Fri, Nov 1, 2013 at 9:11 AM, Vincent Hennebert <[email protected] >> >wrote: >> >> On 01/11/13 16:52, Glenn Adams wrote: >>> >>> On Fri, Nov 1, 2013 at 7:39 AM, Glenn Adams <[email protected]> wrote: >>>> >>>> >>>> On Fri, Nov 1, 2013 at 1:50 AM, Vincent Hennebert < >>>>> [email protected] >>>>> >>>>>> wrote: >>>>>> >>>>> >>>>> We have here a typical example of how code is being made worse due to >>>>> >>>>>> the rigidity of a code checking tool: >>>>>> >>>>> <snip/> > > >>>>>> I suggest you revert this change. >>>>>> >>>>>> >>>>>> No thank you. The original code is broken since it depends on use of >>>>> a >>>>> ClassCastException when it shouldn't. I only fixed this because >>>>> findbugs >>>>> reported this problem and whomever checked in in the first place didn't >>>>> fix >>>>> the warning correctly. >>>>> >>>>> I have recently pointed out on a couple of occasions that people are >>>>> leaving checkstyle or findbugs warnings in the tree and not fixing >>>>> them. >>>>> If >>>>> they don't fix them in the first place, then I'll do it when I do a >>>>> commit >>>>> in order to get a clean tree. If they want to go back and revise my >>>>> fix, >>>>> that's fine with me as long as they don't leave warnings lying around. >>>>> >>>>> >>>>> To elaborate to make sure I'm understood: >>>> >>>> If anyone commits a change that introduces a new checkbug or findbug >>>> warning, or breaks an existing junit test, then that person is >>>> responsible >>>> for fixing the problem ASAP. If they fail to do so, then whatever >>>> fix-ups >>>> get applied by other committers is acceptable provided they don't >>>> introduce >>>> other new warnings or break existing tests. >>>> >>>> This is just SOP. >>>> >>>> >>> We do have a no Checkstyle warning policy after the revised Checkstyle >>> configuration file we agreed on some time ago. >>> >>> However, we have never made a similar agreement on FindBugs, precisely >>> because people couldn’t agree on the interest of this tool due to the >>> high ratio of noise (at least in its standard configuration). >>> >>> If you want to use FindBugs on your local copy, then you’re absolutely >>> free to do so, but please don’t impose it on other people without first >>> reaching an agreement about it. >>> >>> >> I beg to differ. We have been operating on a no warnings policy for >> findbugs as long as I've been with the project. >> > > No we haven’t. We had a discussion a few years ago about introducing > such a policy, but people were not excited about it and no vote > occurred: > http://markmail.org/message/**taztvj2ms7x6tryb<http://markmail.org/message/taztvj2ms7x6tryb> > > > > The fact that folks are >> keeping it up to date (without warnings) in general proves that is the >> case. >> > > It just shows that more than one developers informally use FindBugs, > that’s all. > > As far as I’m concerned, I find the amount of false positives (in its > stock configuration) an annoyance rather than a help. And, in the > present case, counter-productive. There's an easy fix here Vincent. Just start running and fixing findbugs warnings like the rest of us. Then you won't have to complain about me fixing your findbugs warnings in a way you don't like. Yes, it is a bit annoying, but I find it occasionally catches real bugs. > > > > IMO your change is not acceptable because it makes the code less >>> future-proof (less robust to future changes). Which to me is at least as >>> important as introducing warnings or breaking tests. >>> >> >> >> So fix it in the way you like that doesn't introduce a findbugs warning. >> > > > Vincent >
