Skip to content
William Stein edited this page Sep 6, 2018 · 6 revisions

CoCalc Pull Review Checklist

  • does include step-by-step test instructions.
  • does solve the problem it claims to
  • does NOT introduce new bugs (unless there is a mitigation plan)
  • does NOT introduces a denial of service attack vector or security vulnerability
  • does NOT contains any suspicious TODO (unless there is a mitigation plan)
  • does NOT contains significant new code in CoffeeScript; just put that code in typescript in another file and require it.
  • does NOT explicitly uses Promises when async/await could be used instead. (We already use this callback library all over, and we use async/await. The last thing we need is doing async three different ways all over the place.)
  • does NOT use foo! in TypeScript to avoid null checks; instead do if (foo == null) {...} else {...}.
  • does NOT silently ignore errors

Other things to worry about (depending on the code):

  • does work on our supported browsers
  • does NOT confuse users too much (e.g., if you add a new option do NOT have it move another common option out of the way -- instead put your new option below; it's probably not that frequently used of an option if it hasn't been implemented until now).

Definitions:

A bug is by definition something that is "unexpected behavior" for most users.

Clone this wiki locally