Being more kind in PR reviews?

[pmoravec]

In the past few weeks, I was thinking over and over how to express my plea using my clumsy English, to get more positive than negative impact of my message and I am still uncertain how to deliver my thoughts. Anyway here I am.

There were cases where the PR review process, how it was done, demotivated the PR authors and redundantly delayed adoption of new features and fixes. Here is my plea to PR reviewers (including myself):

• Review the intention or justification of the PR first, then do the technical review of the code. Reversed ordering does not look professional.
• Limit the number of iterations requesting to change the PR. Every author is willing to do some limited number of PR respins, so do not waste the attempts redundantly:
  • Try to identify all required changes within one review. If you have to interrupt your review, maybe it's worth notifying the author that the review will continue?
  • Minimize finding of new suggestions to improve the code you already reviewed in previous iteration(s).
  • Let’s be (more) kind to PR authors. The requirements we put on them are pretty strict and being pedantic on all aspects can easily demotivate them. They are humans that want to contribute to the project, so don’t disgust their willingness to help. Is the code quite good in its 4th respin, but still you have some nitpicks? Let's accept the PR. Do you prefer some snippet to be in a higher quality? Let's raise our own PR later on, or apply the changes during merge (with notifying the author).

In the past few months, there were several PRs/discussions/issues where we could have done better, several/multiple of us, including myself. As I don't want to fall to a personal level, I don't want to pinpoint individual issues or comments. If you realized you could do better, great. If not, maybe you already behave accordingly :).

[arif-ali]

I agree wholeheartedly, and I was the reason the previous PR that got abandoned.

Improvements are always good, and hence I was looking at the labelling, such that of a review was required, we could see this in glance. It was my bad at the time, that I came late into the review and hence upset the PR author, which was not my intention.

I like your last bullet point, if they are extremely minor nitpicks, and it upsets us, then we should fix them as part of merging, and I don't have a problem with that personally.

Always good to make improvements, all on board