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

On merge requests

By krr

On merge requests

  • 1,190