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

[Anton Khirnov]

Quoting Vittorio Giovara (2023-08-25 19:26:21)
> On Fri, Aug 25, 2023 at 5:24 PM Anton Khirnov <anton at khirnov.net> wrote:
> 
> > Quoting Rémi Denis-Courmont (2023-08-25 17:09:55)
> > > Le perjantaina 25. elokuuta 2023, 17.58.40 EEST Anton Khirnov a écrit :
> > > > > And then sometimes an argument has been argued to death previously
> > and
> > > > > there is really no point to rehash it again and again. If people
> > cannot
> > > > > agree, they should refer to the TC, not brute force the review
> > through
> > > > > overwhelming insistance.
> > > >
> > > > I think we just have different interpretations of the word
> > > > 'constructive' here.
> > > > I certainly agree that some patches are just not acceptable - I
> > certainly
> > > > did not mean to imply that there must be a way forward for all patches.
> > >
> > > I think that you do not agree with the generally accepted meaning of
> > > "constructive" in this context. By definition a review cannot be
> > constructive,
> > > as in helpful or conducive of a way forward, if it argues that there are
> > no
> > > ways forward.
> >
> > Explaining why a patch is not acceptable is helpful IMO.
> > Saying 'no', on the other hand, is not.
> >
> 
> that is true, but saying "no" and preventing some bad code from making it
> in the codebase is better than not saying anything

If the code is so bad that it should not go in then surely someone can
find it in themselves to write two sentences about the reason why it is
so bad. Nobody is saying you have to produce a 10-page manifesto.

> > Maybe you meant "supported" or "corroborated".
> >
> > Might as well describe it in more than one word, since apparently it's
> > so unclear. Would you be in favor of something along the lines of
> >
> >   Nontrivial (i.e. other than cosmetics or accepting the patch) reviews
> >   must be based on technical arguments. If the reviewer fails to provide
> >   arguments for rejecting the patch or requesting changes, then the
> >   review may be disregarded.
> >
> 
> I agree with the text suggested, but I don't understand why it needs to be
> set in stone in the first place...

There is a persistent problem with certain people rejecting patches for
no clear reason, and then refusing to elaborate.

-- 
Anton Khirnov