| Message ID | 20230824195615.19017-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | doc/developer patch review improvements | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Thu, Aug 24, 2023 at 9:56 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > 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 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 offlist and irc discussion is of course recommended, but writing this rules in stone will only deter good reviews, in my opinion
Vittorio Giovara (12023-08-25): > NAK > we shouldn't put extra burden on reviewers, nor guilt trap them into > suggesting an alternative approach It is hilarious, in a very sad way, that you prefer put extra burden on people who do things than on people who block them.
On Fri, Aug 25, 2023 at 8:46 AM Nicolas George <george@nsup.org> wrote: > Vittorio Giovara (12023-08-25): > > NAK > > we shouldn't put extra burden on reviewers, nor guilt trap them into > > suggesting an alternative approach > > It is hilarious, in a very sad way, that you prefer put extra burden on > people who do things than on people who block them. > It is easier to post dubious solutions instead of doing proper research or/and review. > > -- > Nicolas George > _______________________________________________ > 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". >
Quoting Vittorio Giovara (2023-08-25 03:56:44) > On Thu, Aug 24, 2023 at 9:56 PM Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > 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 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.
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. 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.
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?
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. Maybe you meant "supported" or "corroborated".
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. > 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.
On Fri, Aug 25, 2023 at 8:46 AM Nicolas George <george@nsup.org> wrote: > Vittorio Giovara (12023-08-25): > > NAK > > we shouldn't put extra burden on reviewers, nor guilt trap them into > > suggesting an alternative approach > > It is hilarious, in a very sad way, that you prefer put extra burden on > people who do things than on people who block them. > I am confused, your email is the epitome of not suggesting any alternative approach, so you're agreeing with it..?
On Fri, Aug 25, 2023 at 5:24 PM Anton Khirnov <anton@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...
On 8/25/23 11:09, Rémi Denis-Courmont wrote: > 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. > > Maybe you meant "supported" or "corroborated". > FWIW I read it the same way Anton did but if it's unclear then perhaps it could be modified. Essentially, I think what's going on is we don't want "NAK" without a reason. If you want to say a patch shouldn't make it in, there should at least be a reason. Even if the reason is "this API/module has no place in FFmpeg." - Leo Izen (Traneptora)
Quoting Vittorio Giovara (2023-08-25 19:26:21) > On Fri, Aug 25, 2023 at 5:24 PM Anton Khirnov <anton@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.
Leo Izen (12023-08-25): > FWIW I read it the same way Anton did but if it's unclear then perhaps it > could be modified. Essentially, I think what's going on is we don't want > "NAK" without a reason. If you want to say a patch shouldn't make it in, > there should at least be a reason. I agree on this too. > Even if the reason is "this API/module has no place in FFmpeg." But not on this example: what has place in FFmpeg or not is anybody's arbitrary opinion, saying “no place in FFmpeg” alone is just a fancy way of saying “NAK” with no reason. It must be substantiated too, for example “the same feature is already possible [like that]”. And if the same feature is *not* already possible, then it surely means the code *does* belong in FFmpeg. Regards,
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. + @anchor{Regression tests} @chapter Regression tests
Suggested text is from Anton Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- doc/developer.texi | 3 +++ 1 file changed, 3 insertions(+)