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

[Anton Khirnov <anton@khirnov.net>]

Quoting Rémi Denis-Courmont (2023-08-25 16:22:45)
> Le torstaina 24. elokuuta 2023, 22.56.14 EEST Michael Niedermayer a écrit :
> > Suggested text is from Anton
> > 
> > Signed-off-by: Michael Niedermayer <michael@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
> 
> Well, frankly, no. You're really confusing code reviews with teaching here. A 
> code review is first and foremost meant to find problems and avoid adding bugs 
> or bad designs into the code base. It is not meant to solve problems. Of 
> course, it is preferable for a review to be constructive, but it is not always 
> possible or reasonable.
> 
> Sometimes code just does not belong in.
> 
> Sometimes the reviewer can prove or make strong arguments that a patch is 
> broken or bad, without having constructive feedback to give. This is just like 
> mathematical proofs. Some are constructive, some aren't.
> 
> 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. The intent is rather that every review (other
than OK) should be based on technical arguments and not be a semantic
equivalent of 'no'. In case you did not notice, we have a persistent
problem with some people who are sending "reviews" of exactly this type.
I don't think that should be acceptable.

Would you be happier with some reformulation of the text that makes this
more clear?

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".