pr

CoCalc Pull Review Checklist

• does include step-by-step test instructions.
• does solve the problem it claims to
• does NOT knowingly 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 work in our ways of running CoCalc: dev in a project, on Kubernetes, and in Docker.
• 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).
• does NOT introduce performance regressions (unless there is a good excuse for doing so, or mitigation plan).

Guidelines:

• do NOT leave code sitting open and not in needs-review for a long time. This is bad for development, since it discourages others from working on the same part of the code or addressing the important issue that you were supposed to be fixing.
• do many smaller PR's, instead of big ones.
• do get your code in even before it is ready to be generally user facing, by putting it behind a feature gate. All the caveats above about "unless there is a plan" make this easier. Think of how Chrome or Kubernetes are developed with tons of alpha functionality shipped in production, but hidden behind feature flags.

Definitions:

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