On reviewing Merge Requests (MR)

Some fundamentals

Disclaimer

probably nothing new
but good to summarize

Disclaimer 2
I personally violate these principles way too often.

Learn from my mistakes ;)

Let me know if I am missing some things here

Goal of MR review

Get good and useful code in master

Keep bad and useless code out

Code is good if

• bug-free
• readable
• well-designed
• well-documented
• well-tested
• well-styled (see style guide)

Code is useful if

• improves "goodness"
  • see previous slide
  • bugfixes are almost always good
• implements a useful/required feature
  • corollary: unneeded features should not be implemented
  • corollary 2: what is useful/required is often up for debate and subject to change

MR review: interplay between author and reviewer

• Both have responsibility
• Author: make it easy for reviewer
  • Barts mail
  • e.g. blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/
• Reviewer: focus on the essentials
  • e.g. fuelcycle.org/kernel/pr_review.html
  • some personal caveats (see end of presentation)

MR reviews should be swift

Problem with long-standing MRs:

 

• bugs stay unfixed
• implemented features stay out of master
• new features are not started
• new features are started on old design (merge conflict overhead)
• chains of rebasing for old merge requests
  • also holds for rejected MRs!

 

Hence: focus on essentials

Some reviewing caveats

• Keep feature requests out of MR review
  • make feature request instead
• if you see a code improvement, make an issue, or better yet, code it ;)
  • as extra commit to existing MR or as new MR
• ignore code preferences
• note context: company delivering products under contract vs. experimental research tool
• stay friendly
  • goal is to write great code, not to win arguments