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

[Vittorio Giovara]

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


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