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?
- A process by which code is reviewed. Obviously.
- Peer-review of code before it becomes "official."
- A chance for community involvement.
-
A time for teaching.
- "You can get rid of this for-loop and do this much more elegantly."
-
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!"
Advantages
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:
- Emoji can be used to inject some fun into comments and commit messages.
- http://www.emoji-cheat-sheet.com/ has a cheat-sheet for all possible icons.
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).