Code Review

for Fedora Infrastrcture applications

Created by Ricky Elrod / @relrod6 / codeblock / relrod

About me

  • Intern at Red Hat on the Fedora Engineering team.
  • Involved with Fedora since early summer of 2010.
  • Contributor to Facebook's Phabricator code review toolset.
    • But won't be talking about that here.

What is code review?

  1. A process by which code is reviewed. Obviously.
  2. Peer-review of code before it becomes "official."
  3. A chance for community involvement.
  4. A time for teaching.
    • "You can get rid of this for-loop and do this much more elegantly."
  5. A time for learning.
    • "This looks great - what do you call this pattern?"
    • "Oh wow, that's a much nicer way to do that. I'll remember that from now on!"


Isn't it just more boring process?
  • Encourages community envolvement.
  • Higher quality patches, the first time.
  • Prevents bugs and security holes.
  • Keeps the codebase consistent and clean.
  • Keeps really bad ideas out of the codebase.
  • It can be fun!

As a rule...

You are far more likely to fix code before it is merged into the codebase than after.

There is rarely one solution

...but there is often a better solution than others.

  • It's a very natural mistake to be handed a problem, think of a solution, and assume that it is the solution.
  • Often times, the coder's solution will work, but it's not the best solution for various reasons.

How it works (general)

  • There exists a repository (git, svn, etc.).
  • Someone wants to change a piece of code in that repository.
  • They write a patch.
  • They submit the patch for the community to review.
  • They iterate on the patch with comments from community members.
  • The patch gets merged into the repository.

How it works (Fedora style!)

  • There exists a repository (git).
  • Someone wants to change a piece of code in that repository.
  • They fork the repository (unless they have commit access).
    • This is the only step that people with commit access should skip in most cases - forking.
  • They make a new branch, usually via git flow (out of the scope of this talk).
  • They push the branch to their fork (or the repository itself, if a committer).
  • They send a pull request.
  • They iterate on the patch with comments from community members.
  • The patch gets merged into the repository.

How do I review code?

  • Find an open review/pull request.
  • Look over the changes. Some things to look for:
    • Is this change within the scope of the project?
    • Does this change break existing functionality?
    • Is this change documented well?
    • Is there a better way to implement this idea?
    • Are there any security concerns?
    • Licensing concerns?
  • Run tests, linters, etc.
  • Comment on it, politely listing issues you have found.
  • Discuss alternative/better methods of doing things.
  • Ask for clarification.

Don't be afraid to say nothing.

  • Look for problems and report them or ask for clarification.
  • But if it looks good, there's nothing wrong with just saying "LGTM".
  • (LGTM = Looks Good To Me)

Be prompt, but not rushed.

  • When you see a review request waiting, process it as soon as you have a chance.
  • Don't drop everything you're doing, but the sooner the better.
  • Make it your spare time task: When one might check social networking sites or email, an engineer should seek to clear out their code review queue.
  • Try to give feedback on all pending reviews within 24 hours, if not sooner.
  • That said, don't rush through it. Take your time.

What should a review request look like?

  • Keep it small!
  • One change per Pull Request, if possible.

Having your code reviewed.

  • First off, don't be offended!
  • A main point of code review is to learn new, better ways to do things. Take the opportunity to learn!
  • Never be afraid to ask for clarification, or to defend something you've written.
  • If you can't decide, with the reviewer, the best way to do something, ask other community members.
  • Once you fix the issues, you can amend the commit to your branch, and git push -f

Step 1: Make a change/Submit PR.

Step 2: Wait for comments.

Step 3: Iterate based on comments.

Step 4: Merge or wait for it to be merged.

Eat a lot of :cake:

Thumbs up! :+1:

In summary:

  • Fedora Infrastructure uses pull requests as a form of code review.
  • Code review catches many bugs and flaws that would otherwise go live.
  • Code review can be fun and you can use things like emoji to make it less boring.
  • Ask for clarification - both of the developer and the reviewer.
  • Be prompt, so people aren't blocking on you.
  • Automate what you can (tests, linters, etc).



Works Cited

Thank you!