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
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.
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
