Require 24 hour minimum time for PR reviews

[nyalldawson]

QGIS Enhancement: Require 24 hour minimum time for PR reviews

Date 2024/10/24

Author Nyall Dawson

Contact nyall dot dawson at gmail dot com

Summary

This QEP proposes a change to policy to permit pull request merging only after a minimum 24 hour open-for-review period. (Currently, a PR can be merged immediately, as soon as a core developer has approved the PR)

This QEP is part of a series of proposals designed to improve the QGIS development workflow and document existing policies.

Proposed Solution

Currently, for an developer with commit rights to the QGIS repo to get a pull request merged, they must:

1. Ensure that the PR passes all reasonable CI checks (some exceptions exist to this rule, eg when a check is broken for unrelated reasons (such as broken third party services, or a lint/code analysis/spell check test failing from other parts of a modified file).
2. The PR must be approved by a different developer with core commit rights

This proposal would add an additional policy:

3. A PR must remain open for at least 24 hours following submission, even if it has already been approved. This is to allow wider feedback to be gathered prior to merge, and to permit pre-merge feedback from developers in other time zones. Exceptions to this policy are:
   • Approved pull requests for backports to stable branches
   • "Emergency" pull requests, eg those which fix broken master builds, CI infrastructure or which represent a time-sensitive security risk
   • Trivial fixes. The definition of "trivial" is left open to common sense and developer discretion, but is expected to include non-risky changes like typo fixes, translation string fixes, tab order changes, or similar.

Possible workflow

We could possibly use the "minimum open time" GitHub action to enforce this policy. See https://github.com/gregsdennis/minimum-open-time.

An example repo using this action is https://github.com/json-schema-org/json-schema-spec

[rouault]

I've created a doc pull request capturing this: qgis/QGIS-Documentation#9300