> > You can disable inspection for a warning that is otherwise benign... >
Is there a consensus on what constitutes a benign warning? I think the only times I use @SuppressWarnings is in parameterized tests to suppress the unused method warning for the parameters method, or for unchecked casts, as below: PartitionedRegion detailRegion1 = mock(PartitionedRegion.class); when(cache.getRegion(regionPath1)).thenReturn(detailRegion1); where the thenReturn() would complain, since it's expecting to return a Region<Object, Object>. Would these be considered things that could safely just be ignored (and so for which I can turn off inspection), or is the use of @SuppressWarnings here warranted? On Fri, May 8, 2020 at 11:58 AM John Blum <jb...@pivotal.io> wrote: > Let's try this again :P. > > +1 to Kirk's comments. Plus... > > Another tip (for IJ IDEA users, probably same for Eclipse and other IDEs): > > You can disable inspection for a warning that is otherwise benign (or not > correct) *rather than* unnecessarily annotating the code with > @SuppressWarnings. > > However, disable inspections judiciously. Warnings in code are telling you > something that needs to be paid attention to and disabling the warning with > (@SuppressWarnings) or disabling the inspection makes it easy to lose sight > that the warning is there when the code is revisited. > > -j > > > On Fri, May 8, 2020 at 11:55 AM Jacob Barrett <jbarr...@pivotal.io> wrote: > > > I submitted and PR a while ago that started making warnings errors on > > certain modules. Through lazy consensus it was approved and merged. I > would > > love to apply it to more. To set a baseline, I tried to fix most of the > > deprecated and other warnings as possible in the effected code. Many were > > so bad off that to just keep from getting worse I marked some > suppressions. > > I tried to isolate to single statements, often by extracting the > offending > > line out into its own method and annotating it. > > > > Prior to this almost ever bit of free space in the warning/error gutter > in > > IntelliJ was take up by these warnings. The hope was by making them > errors, > > fixing most and suppressing where necessary that it wouldn’t get any > worse. > > It was hoped it would be signal to the next person in there deprecating > > something, using generics, or whatever, to do it right and clean up the > > effected code too. Just suppressing errors because you want to get it > done > > is not a good practice. Effort should be taken and only as a last resort > > should something be suppressed That suppression should be limited to the > > smallest possible set of statements, which may mean extracting a bit fo > > code into a method. > > > > We most definitely should make it so we don’t add anymore suppressions > but > > absolutely not at the expense of allowing warnings. The code should be > > fixed or refactored to remove the suppression. > > > > -Jake > > > > > > > On May 8, 2020, at 11:45 AM, Kirk Lund <kl...@apache.org> wrote: > > > > > > I'm reviewing lots of PRs which are > > > adding @SuppressWarnings("deprecation"), etc. I think this is a bad > > trend. > > > We should not be suppressing warnings in our code. That's hiding a > > problem > > > that we'll need to or want to deal with in the future. Also, if you add > > > several of these suppress annotations into a class or a method or a > test, > > > it really diminishes the readability. It adds noise and suppresses > valid > > > warnings. > > > > > > On one of Jinmei's PRs, she responded saying she has to add these to > her > > > code because of some change that Jake made to the build. Can we make it > > so > > > we don't have to add these suppressions? > > > > > > Thanks, > > > Kirk > > > > > > -- > -John > Spring Data Team >