Code Review Guidelines
A good pull request:
- Does about one thing (new feature, bug fix, etc)
- Adds tests and documentation for any new functionality
- When fixing a bug, adds or fixes test that would have caught said bug
OCaml things
- Are the style guidelines being followed?
- Do the signatures make sense? Are they minimal and reusable?
- Does anything need to be functored over?
- Are there any error cases that aren't handled correctly?
- Are calls to
_exn
functions justified? Are their preconditions for not throwing an exception met? Is the exception it throws useful? - There shouldn't be commented out code.
- No stray debug code lying around.
- Any logging is appropriate. All
Logger.trace
logs should be inessential, because they won't be shown to anyone by default. - Should this code live in its library? Should it live in a different library?
- Does the code confuse you? Maybe there should be a comment, or it should be structured differently.
- Does a behavior change break assumptions other code makes?