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

[Rémi Denis-Courmont]

Le torstaina 24. elokuuta 2023, 22.56.14 EEST Michael Niedermayer a écrit :
> 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

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.

Also what Vittorio already pointed out. Ultimately, this is also a question of 
what to optimise for. And in my 20+ years experience with software development 
and open-source, I think it's abundantly clear that available skilled 
reviewers are an ever scarcer resource than skilled available developers, so 
you should not optimise for the later.

So -1 as far as I am concerned.

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/