Code review guide

Including an illustrated collection of what not to do.

This document is a work in progress.

A word to the reviewer

Code review is not easy, and not a quick task. If you feel you've spent more time looking at code than necessary, take your time and look at it some more.

There is a difference between things "looking all right" versus things being actually correct. Take your time to understand what changes, why it's being changed, and what the consequences are. It's easier to update a pull request if necessary, than it is to mitigate the fallout of a sneaky bug.

Both the PR author and other reviewers can make mistakes, too. Our task is to double-, or even triple-check their work, so being diligent is important. If the PR author responds to one of your concerns saying they don't think it's right, make a small experiment to decide which one of you is correct.

A small and incomplete list of things to look out for

• Does the PR align with the direction of the repo?
  • Does it implement something we actually want?
    • Examples (that may lean toward no):
      • https://github.com/esp-rs/esp-hal/pull/1984
        Mostly isolated functionality, I don't expect it to be widely used, there's no reason to include it in the main esp-hal repository.
      • https://github.com/esp-rs/esp-hal/pull/1903
        • Opinion: The example was not written for an existing devkit. If it needs more than a few wires, it's probably not a good example.
        • Opinion 2: the demo is very simple. Is it needed because our documentation is lacking? Examples are more expensive and less flexible than good documentation.
  • Are the changes made according to the contribution guide?
• Does the PR work?
  • CI passes cleanly, of course.
  • Does the PR have merge conflicts?
    • GitHub does not notify the author, ping them!
• Is the PR minimal?
  • Does it only touch, what it needs to touch?
    • A few drive-by changes might be okay, but a half-functional, half-cleanup PR should probably be split up.
• Is the PR documented properly?
  • Are the API changes recorded in changelog?
  • Did the author update the migration guide?
  • Does the added documentation make sense?

    • Please avoid over-using ChatGPT. While it helps, it lies.

• If the PR is a bugfix, does it contain new tests?
  • Changing existing tests should be done carefully.
  • Deleting tests is usually a no-no, except when redundant.
  • Make sure the added tests FAILED before the PR!
• Are the new APIs simple to use, hard to misuse?
  • Example:
    • https://github.com/esp-rs/esp-hal/pull/1908
      This may be easy to use, but it turns out, it's also very easy to misuse because the driver is not thread-safe.
• Is the implementation overcomplicated?
  • Unnecessary indirections, overabstraction, complicated logic all hurt maintainability.
  • Esoteric language constructs should probably be avoided.
• Does the PR have any licensing issues?
  • Are new dependencies compatible?
  • Are new content free to use?
• Suspicious code patterns
  • static variables in a const-generic item:
    • #1330 - looks simple, but it's wrong (there's a single waker regardless of the value of N) and leads to tasks stopping to be polled.