On Fri, Nov 1, 2013 at 10:50 AM, Vincent Hennebert <vhenneb...@gmail.com>wrote:

> On 01/11/13 18:21, Glenn Adams wrote:
>
>> On Fri, Nov 1, 2013 at 9:11 AM, Vincent Hennebert <vhenneb...@gmail.com
>> >wrote:
>>
>>  On 01/11/13 16:52, Glenn Adams wrote:
>>>
>>>  On Fri, Nov 1, 2013 at 7:39 AM, Glenn Adams <gl...@skynav.com> wrote:
>>>>
>>>>
>>>>  On Fri, Nov 1, 2013 at 1:50 AM, Vincent Hennebert <
>>>>> vhenneb...@gmail.com
>>>>>
>>>>>> 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
>

Reply via email to