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