[FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive

[Anton Khirnov]

Quoting Vittorio Giovara (2023-08-25 03:56:44)
> On Thu, Aug 24, 2023 at 9:56 PM Michael Niedermayer <michael at niedermayer.cc>
> wrote:
> 
> > Suggested text is from Anton
> >
> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > ---
> >  doc/developer.texi | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/doc/developer.texi b/doc/developer.texi
> > index 0c2f2cd7d1..383120daaa 100644
> > --- a/doc/developer.texi
> > +++ b/doc/developer.texi
> > @@ -853,6 +853,9 @@ Everyone is welcome to review patches. Also if you are
> > waiting for your patch
> >  to be reviewed, please consider helping to review other patches, that is
> > a great
> >  way to get everyone's patches reviewed sooner.
> >
> > +Reviews must be constructive and when rejecting a patch the reviewer must
> > explain
> > +their reasons and ideally suggest an alternative approach.
> >
> 
> NAK
> we shouldn't put extra burden on reviewers, nor guilt trap them into
> suggesting an alternative approach

I don't understand this argument at all.

First, "ideally suggest an alternative approach" is an aspiration, not a
hard requirement.

Second, I don't think reviewers should be able to reject patches with no
explanation. The author/submitter spent time and effort on writing
and submitting the patch - it is only fair that if it's to be rejected,
it should be done for a clear reason.

> offlist and irc discussion is of course recommended,

I absolutely do not recommend offlist discussion, as it is not visible
to other developers or preserved in the archives.

> but writing this rules in stone will only deter good reviews, in my
> opinion

Non-constructive reviews without an explanation are never good reviews.

-- 
Anton Khirnov