Skip to content

(WIP) Add specific criteria for PR review

homotechsual requested to merge github/fork/seancolsen/pr-review-criteria into master

Created by: seancolsen

Before

The steps to review a core PR give the reviewer quite a bit of leeway to "form an opinion about the fix" without very much guidance on how people should be forming those opinions.

After

Developers can use the following template when reviewing a core PR

  • It's a worthy pursuit.
  • It works as advertised.
  • It makes sense for all users.
  • It's unlikely to surprise users.
  • New code is readable and makes sense.
  • It doesn't appear to introduce security vulnerabilities.
  • Acceptance criteria are either: (a) not stated, or (b) met.
  • Significant new functionality is either: (a) not present, or (b) covered by new unit tests.
  • New documentation is either: (a) not needed, or (b) provided within this PR, or (c) provided in a separate PR which already exists.

Notes

This PR would be somewhat of a policy change (not just a docs improvement), and as such, I would want approval from people like @totten @eileenmcnaughton @seamuslee001 (and maybe others — not sure what the protocol is for making decisions like this).

This was just an idea I had. I won't push it. Very much open to swiftly closing this PR if others are not interested in it!

Merge request reports