[QGIS-Developer] Stale PRs best practice

[Nyall Dawson]

On Thu, 23 Apr 2020 at 19:16, uclaros <uclaros at gmail.com> wrote:
>
> Hi all,
> I would like to ask what would be considered best practice for an issuer to
> handle stale pull requests. In the cases that a review was made and further
> changes are expected from the contributor, it is obvious that unless the
> changes are applied the PR should be closed. My question is about PRs that
> have received no suggestions or no comments whatsoever. I don't want in any
> way to sound pushy, I can see the length of the PR list and I know most (if
> not all) are just donating time to the project, that's why I ask here.
> Should I just close and reopen the PR to reset the stale timer and wait?
> Should I add an "up" comment to reset stale timer or a comment asking for
> attention?
> Should I ask someone specific for a review? (who?)

I'd say:
1. Do everything you can to make the PR easy to review first -- make
sure all tests are passing, the commit log is clean (free of merge
commits and squashed to logic commits), and that there's no merge
conflicts

2. Then, if someone has already reviewed:
    - make sure ALL comments have been resolved. Then do a gentle
"ping @person this is ready for re-review now"
    - if you haven't resolved all outstanding comments (e.g. because
you disagree with something), then:
          a) if it's "just" a question of coding style, API choice, or
use of QGIS/Qt APIs: trust the reviewer. Better to take a step back on
your arguments and make the QGIS maintainers/reviewers happy and see
your work merged, then fight because you think QGIS coding
conventions/established API style is "wrong".
          b) If the reviewer is wrong (e.g. they've misinterpreted
what the change does) you'll need to present your counterarguments
politely (don't get the reviewers off-side! We all make mistakes, but
are MUCH more likely to return to review a PR if the submitter is
polite and working WITH you!)

If no one has reviewed yet, it's probably the case that an initial PR
was submitted in a partially complete state or which was failing units
tests, and we're just not aware that the work is now ready for review.
Do a gentle bump "This PR is ready for review"

Hope that helps!

Nyall