Things to check for in pull requests

CR.md

People submitting PRs:

Prune your diffs:

Strip out -

• spaces and newlines
• commented out code
• console logs
• orphaned code - Did you write a function that never gets called? This problem is much more difficult for people reviewing PRs to catch

Make sure merge-order doesn't matter

• Branch off master/dev, not your previous working branch that you just submitted a pull request for.

1 feature per commit

• When something's broken and you've finished rolling back commits to find which one broke it, it's easier to spot which specific piece of code is the culprit if the commit isn't too huge.
• If the code for one feature is good and the code for another feature has problems, good code can be merged in without waiting for the bad code to be fixed.

Did you break the project?

• Make sure it compiles
• Look at the site
  • check for new error-logs
  • does everything look more or less right?
    I've had a commit that broke the entire site because a meta tag was missing a closing bracket.
• Look at the diff.
  Just a quick 90-second glance pays off.
  I've seen pull requests where merge conflicts where checked in.
  You don't have to go through your code with a fine-toothed comb to spot a green line of <<<<<<< HEAD
• Nice to have: screenshot in PR for UI updates, and comments specifying which pages you can go to to see the changes

People reviewing PRs: Look out for:

• Liar variables/methods/states
  • an example - image vs imageSrc (topMenu, functions named goNext when it's in fact goNextUnlessMobile )
• Orphaned code
• Changing function signatures (especially for async functions, watch out for changes in code flow & "then-able-ness")
• Timeouts.
  • usually hacks, try to find a way to avoid using it
  • make sure that any necessary evils are "cleared"
    (e.g. if you have a timeout in enter, make sure you kill the timeout in enterCancelled)
• overly complex code:
  https://gist.github.com/cheapsteak/4d0321dfcab0c099e676

Things that I don't care too much about when reviewing pull requests (dont think it's worth holding up a merge)

• Styling

Things I'm not sure about

• Squashing commits for pull requests

• Benefits:
  • It makes things more tidy by cutting down the number of junk commits (e.g. "oops missed a comma")
• Costs:
  • Mental overhead
  • Really large commits in the history that can't be displayed