Add "Review Standards"

Created by: totten

Created by: totten

• Ensure that the PR links to a JIRA issue (r-jira)
  • All good
  • WTF
• Examine test results (r-test)
• Read the code (r-read)
• Try it out (r-run)
• Assess impact on users (r-users)
• Assess impact on extensions/integrations (r-ext)
• Assess impact on core (r-core)
• Check for tests or maintainability (r-maint)
• Check for documentation (r-doc)

Created by: mickadoo

Test

• Ensure that the PR links to a JIRA issue (r-jira)
• Examine test results (r-test)
• Read the code (r-read)
• Try it out (r-run)
• Assess impact on users (r-users)
• Assess impact on extensions/integrations (r-ext)
• Assess impact on core (r-core)
• Check for tests or maintainability (r-maint)
• Check for documentation (r-doc)

Created by: seancolsen

@totten Do you think this PR needs more work before we merge it? Or should we go-ahead? Looks good as far as I'm concerned but I'm wondering if you are still thinking of it as WIP.

Created by: totten

I guess it's still WIP.

This may sound nitpicky, but there's a sort of tension in converting those headings to a checklist. They're originally conceived as, "These are steps that a reviewer should go through" -- the intention was that you'd go through all of them and then make a note if any had redflags. But it doesn't translate well into checklist format. (As you pointed out in Devon, the blank-checkbox is ambiguous -- it could mean "Needs to be assessed" or "Failed the check." Similarly, the checked-checkbox is also ambiguous -- it could mean "It passed" or "I did the step of checking it". In either case, it's not clear that you really need to leave comments about what was good or what was bad.)

@seanmadsen I started playing with another rendition of the checklist -- what do you think? https://gist.github.com/totten/283405be2aaaa5c8d533a13c7c08de71

Created by: seancolsen

@totten Cool! Personally, I really like where that gist is going. But I'm also sensitive to the fact that many people voiced concern with the PR review process becoming too bureaucratic. Your latest proposed checklist is quite a bit more logical, but also not quite as simple. I think it's a great idea, but I'd want to see what others think, especially people who are wary of adding too much structure here.

Created by: seancolsen

We could also pull the checklist out of this PR, merge, and then create a separate (or multiple competing) PR(s) to just add the checklist.

Created by: totten

Good idea @seanmadsen.

For the moment, I've pulled out the checklist and can submit that separately. Additionally, I've simplified the headings and expanded some of the discussion.

Created by: seancolsen

Thanks @totten this is great